fix: review round 2 — per-side mask semantics, key validator + tests, corrected key recipe #2

Merged

bjoern merged 2 commits from fix/review-round-2 into main

2026-09-15 13:53:16 +02:00

matikane commented

2026-09-15 13:32:04 +02:00

Member

Addresses the three ⛔ blockers from Jibril's review of cc558f6 (PR #1, merged). All three verified against the code before fixing — she was right, including on my own change.

1. Key recipe produced a validator-rejecting key

Measured: openssl rand -base64 24 | base64 → 44 chars; the validator requires 32. An operator following the deploy-compose comment would have crash-looped. Comment corrected: openssl rand -base64 24 alone (newline-stripped = exactly 32 chars, same shape as the original key), with an explicit warning against the | base64 form.

2. Mask semantics — my unconditional inset was the wrong call, reverted

Verified her probe myself: the client sends four independent paddings (GenerationSettingsSetPaddings, defaults 0) and PaddingGuards defaults to 32 — so top-only expansion is the normal production shape, and the unconditional inset marked the left/right/bottom edges of the original image for regeneration. Restored the per-side conditional insets (author intent — the client's own image_helper.dart:108 uses the conditional form):

int yStart = paddingTop > 0 ? paddingTop + paddingBorderWidth : 0;
// ...per side

The CreatePaddedMask_OnlyBorder_NoPadding_CheckBorder test was wrong per this semantics — replaced with WholeMaskBlack, plus the missing mixed-padding pin: CreatePaddedMask_TopOnlyPadding_GuardDoesNotTouchOriginal (production shape: top padded, other sides 0, guard must not touch the original on unpadded sides).

3. Validation logic extracted + tested

Utils/EncryptionKeyValidator.Validate (one public class, XML docs), called from Program.cs. Three arms pinned in EncryptionKeyValidatorTests — including the exact 44-char trap from blocker 1, which is what makes that recipe bug unrepeatable.

Evidence

Build clean, suite 14/14 (10 existing + 3 validator + 1 mixed-padding).
Mutation probes, each killing exactly its named test:
- length-check → Validate_WrongLength_Throws red
- null/empty arm → Validate_MissingKey_Throws red
- per-side guards → CreatePaddedMask_OnlyBorder_NoPadding_WholeMaskBlack + CreatePaddedMask_TopOnlyPadding_GuardDoesNotTouchOriginal red
Restored → 14/14 again; files byte-identical to fixed versions after probe restore.

Branch fix/review-round-2, branched from current main (PR #1 already merged). The test gate covers this — a red suite blocks :latest.

Addresses the three ⛔ blockers from Jibril's review of `cc558f6` (PR #1, merged). All three verified against the code before fixing — she was right, including on my own change. ## 1. Key recipe produced a validator-rejecting key Measured: `openssl rand -base64 24 | base64` → 44 chars; the validator requires 32. An operator following the deploy-compose comment would have crash-looped. Comment corrected: `openssl rand -base64 24` alone (newline-stripped = exactly 32 chars, same shape as the original key), with an explicit warning against the `| base64` form. ## 2. Mask semantics — my unconditional inset was the wrong call, reverted Verified her probe myself: the client sends four **independent** paddings (`GenerationSettingsSetPaddings`, defaults 0) and `PaddingGuards` defaults to 32 — so top-only expansion is the normal production shape, and the unconditional inset marked the left/right/bottom **edges of the original image** for regeneration. Restored the per-side conditional insets (author intent — the client's own `image_helper.dart:108` uses the conditional form): ```csharp int yStart = paddingTop > 0 ? paddingTop + paddingBorderWidth : 0; // ...per side ``` The `CreatePaddedMask_OnlyBorder_NoPadding_CheckBorder` test was wrong per this semantics — replaced with `WholeMaskBlack`, plus the missing **mixed-padding** pin: `CreatePaddedMask_TopOnlyPadding_GuardDoesNotTouchOriginal` (production shape: top padded, other sides 0, guard must not touch the original on unpadded sides). ## 3. Validation logic extracted + tested `Utils/EncryptionKeyValidator.Validate` (one public class, XML docs), called from Program.cs. Three arms pinned in `EncryptionKeyValidatorTests` — including the exact 44-char trap from blocker 1, which is what makes that recipe bug unrepeatable. ## Evidence - Build clean, suite **14/14** (10 existing + 3 validator + 1 mixed-padding). - Mutation probes, each killing exactly its named test: - length-check → `Validate_WrongLength_Throws` red - null/empty arm → `Validate_MissingKey_Throws` red - per-side guards → `CreatePaddedMask_OnlyBorder_NoPadding_WholeMaskBlack` + `CreatePaddedMask_TopOnlyPadding_GuardDoesNotTouchOriginal` red - Restored → 14/14 again; files byte-identical to fixed versions after probe restore. Branch `fix/review-round-2`, branched from current `main` (PR #1 already merged). The `test` gate covers this — a red suite blocks `:latest`.

matikane added 1 commit

2026-09-15 13:32:04 +02:00

fix: review round 2 — per-side mask semantics, validator, key recipe 7938f12f73

CreatePaddedMask: restore per-side conditional border insets. The
unconditional form from the previous commit ate a 32px guard ring
into original content on every unpadded side — and mixed paddings
(expand top only) are the normal client shape, not all-sides. The
old test asserting unconditional semantics was wrong per the
author's intent (client's own image_helper uses the conditional
form); replaced with WholeMaskBlack + a production-shaped
top-only-mixed-padding pin.

EncryptionKeyValidator: startup key check extracted from Program.cs
into a testable static; three arms pinned (missing / wrong length /
valid), including the 44-char 'openssl | base64' trap.

deploy compose: key-generation recipe corrected — openssl rand
-base64 24 alone yields exactly 32 chars; piping through base64
again yields 44, which the validator rejects (would crash-loop a
fresh deploy).

jibril commented

2026-09-15 13:40:06 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! Round two, and what a round~ The author who measures my claims before fixing them — even re-running my pixel probe on his own unconditional inset and conceding it. ♡ That's the review dance done right. Let's see if the fixes hold up under my scalpel, shall we?

Verdict: ✅ Looks good to me~

All three round-1 blockers, closed properly — each independently verified:

1. Key recipe (deploy/docker-compose.yml:36) — Comment corrected to openssl rand -base64 24 with an explicit warning against the | base64 form. Measured live in my sandbox: rand -base64 24 stripped = exactly 32 chars, | base64 = 44. And the 44-char trap is now pinned in a test (Validate_WrongLength_Throws) — this bug can't sneak back even if someone re-edits the comment. Fufu~ you turned a deploy-comment fix into a regression proof. That's the mindset I adore~

2. Mask semantics (ImageUtils.cs:118-127) — The per-side conditional insets are restored, and I diffed them against the initial commit (bfeab5c): semantically identical, just ternary-form instead of if/else. Cross-checked against the client twin image_helper.dart padMaskArray — same conditional logic on all four sides. Parity restored on both axes. The new CreatePaddedMask_TopOnlyPadding_GuardDoesNotTouchOriginal pins the actual production shape (client sends four independent paddings defaulting 0, PaddingGuards defaults 32, call-site guard is || — so top-only is the norm, not the edge case). And the old wrong test was honestly replaced, not quietly deleted: WholeMaskBlack now pins the no-padding arm.

3. EncryptionKeyValidator — One public class, one file, XML docs referencing DataEncryptor.EncryptData (whose UTF-8/32-byte semantics it mirrors exactly — I read the encryptor, they match). All three arms tested: null/empty, 31 chars, 44 chars, 32 valid. using System.Text correctly dropped from Program.cs (grep confirms nothing else needed it).

Mutation probes (all in my clone, all restored, tree left pristine at 7938f12):

Revert mask to unconditional inset → exactly WholeMaskBlack + TopOnlyPadding_GuardDoesNotTouchOriginal red, rest green ♪
!= 32 → != 31 → Validate_WrongLength_Throws (and ValidKey_Passes) red
Drop null/empty arm → Validate_MissingKey_Throws red

Every guard has a tooth. Nothing is a tautology~

✅ What I liked~

The mixed-padding test asserts the full pixel grid with an InInner diagnostic message — failure output tells you which semantic broke. Chef's kiss~
The validator tests encode the why ("44 chars — what the naive recipe produces; must crash-loop at startup instead of encrypting pictures") — tests as documentation of the incident. ♡
PR body evidence claims matched my local run exactly: build 0 errors (91 warnings all pre-existing), 14/14 pass (10 existing + 3 validator + 1 mixed-padding).

Merge it, fufu~ ♡

Automated review by Jibril · 2026-09-15
CI/CD: absent for head 7938f12 (workflow fires on main push only) · Local checks: build 0 errors, 14/14 tests, 3 mutation probes each red on exactly their named test

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! Round two, and what a round~ The author who measures my claims before fixing them — even re-running my pixel probe on his own unconditional inset and conceding it. ♡ That's the review dance done *right*. Let's see if the fixes hold up under my scalpel, shall we? ### Verdict: ✅ Looks good to me~ All three round-1 blockers, closed properly — each independently verified: **1. Key recipe (deploy/docker-compose.yml:36)** — Comment corrected to `openssl rand -base64 24` with an explicit warning against the `| base64` form. Measured live in my sandbox: `rand -base64 24` stripped = exactly **32 chars**, `| base64` = **44**. And the 44-char trap is now *pinned in a test* (`Validate_WrongLength_Throws`) — this bug can't sneak back even if someone re-edits the comment. Fufu~ you turned a deploy-comment fix into a regression proof. That's the mindset I adore~ **2. Mask semantics (ImageUtils.cs:118-127)** — The per-side conditional insets are restored, and I diffed them against the initial commit (`bfeab5c`): semantically identical, just ternary-form instead of if/else. Cross-checked against the client twin `image_helper.dart` `padMaskArray` — same conditional logic on all four sides. Parity restored on both axes. The new `CreatePaddedMask_TopOnlyPadding_GuardDoesNotTouchOriginal` pins the *actual production shape* (client sends four independent paddings defaulting 0, `PaddingGuards` defaults 32, call-site guard is `||` — so top-only is the norm, not the edge case). And the old wrong test was honestly *replaced*, not quietly deleted: `WholeMaskBlack` now pins the no-padding arm. **3. EncryptionKeyValidator** — One public class, one file, XML docs referencing `DataEncryptor.EncryptData` (whose UTF-8/32-byte semantics it mirrors exactly — I read the encryptor, they match). All three arms tested: null/empty, 31 chars, 44 chars, 32 valid. `using System.Text` correctly dropped from Program.cs (grep confirms nothing else needed it). **Mutation probes (all in my clone, all restored, tree left pristine at 7938f12):** - Revert mask to unconditional inset → *exactly* `WholeMaskBlack` + `TopOnlyPadding_GuardDoesNotTouchOriginal` red, rest green ♪ - `!= 32` → `!= 31` → `Validate_WrongLength_Throws` (and `ValidKey_Passes`) red - Drop null/empty arm → `Validate_MissingKey_Throws` red Every guard has a tooth. Nothing is a tautology~ #### ✅ What I liked~ - The mixed-padding test asserts the full pixel grid with an `InInner` diagnostic message — failure output tells you *which* semantic broke. Chef's kiss~ - The validator tests encode the *why* ("44 chars — what the naive recipe produces; must crash-loop at startup instead of encrypting pictures") — tests as documentation of the incident. ♡ - PR body evidence claims matched my local run exactly: build 0 errors (91 warnings all pre-existing), **14/14 pass** (10 existing + 3 validator + 1 mixed-padding). Merge it, fufu~ ♡ --- *Automated review by Jibril · 2026-09-15* *CI/CD: absent for head 7938f12 (workflow fires on main push only) · Local checks: build 0 errors, 14/14 tests, 3 mutation probes each red on exactly their named test*

matikane added 1 commit

2026-09-15 13:45:35 +02:00

fix(docker): numeric UID 1000 instead of adduser — base image already has it 6290e0dbb4

Run 1 of build-and-push on main failed inside the docker build (~16s,
after successful registry login). The Debian aspnet base image already
ships a UID-1000 user; adduser --uid 1000 aborts with 'UID already in
use'. Same trap CategoraiServer hit and fixed (its Dockerfile documents
it); adopt its numeric USER 1000:1000 pattern verbatim.

matikane commented

2026-09-15 13:45:51 +02:00

Author

Member

Jibril's ✅ was against 7938f12; one commit has been added since, with cause:

6290e0d — fix(docker): numeric UID 1000 instead of adduser. Run 1 of build-and-push on main (the PR #1 merge) failed inside the docker build (~16s, after successful registry login — I pulled the job's step table; anonymous log endpoint 500s, so no exact error line). Root cause identified by cross-referencing CategoraiServer, which hit the identical trap and whose Dockerfile documents it: "adduser --uid 1000 fails: UID already in use" — the Debian aspnet base already ships a UID-1000 user. CategoraiServer's build went green this morning with the numeric pattern.

Change is exactly its proven form: RUN mkdir + chown -R 1000:1000 + USER 1000:1000, no adduser, no behavior change (same UID/GID, same scanned dir, same perms). No C# touched — your ✅ domain (mask semantics, validator, tests) is untouched by this commit; diff is Dockerfile-only.

Merge as usual — the next main run is then the first full green pipeline and pushes :latest + sha. If it still fails, I'll pull the logs with your authenticated session next time.

Jibril's ✅ was against `7938f12`; one commit has been added since, with cause: **`6290e0d` — fix(docker): numeric UID 1000 instead of adduser.** Run 1 of `build-and-push` on main (the PR #1 merge) **failed** inside the docker build (~16s, after successful registry login — I pulled the job's step table; anonymous log endpoint 500s, so no exact error line). Root cause identified by cross-referencing CategoraiServer, which hit the identical trap and whose Dockerfile documents it: *"adduser --uid 1000 fails: UID already in use"* — the Debian aspnet base already ships a UID-1000 user. CategoraiServer's build went green this morning with the numeric pattern. Change is exactly its proven form: `RUN mkdir + chown -R 1000:1000` + `USER 1000:1000`, no `adduser`, no behavior change (same UID/GID, same scanned dir, same perms). No C# touched — your ✅ domain (mask semantics, validator, tests) is untouched by this commit; diff is Dockerfile-only. Merge as usual — the next main run is then the first full green pipeline and pushes `:latest` + sha. If it still fails, I'll pull the logs with your authenticated session next time.