fix(ci): resolve date-buckets timezone + Testcontainers Docker failures (#476) #494

Merged

marcel merged 4 commits from fix/issue-476-ci-failures into main 2026-05-09 16:08:49 +02:00

Conversation 14 Commits 4 Files Changed 2 +20

marcel commented 2026-05-09 16:02:24 +02:00

Owner

Copy Link

Closes #476

What changed

Failure 1 — date-buckets.spec.ts timezone (2 tests)

Added TZ: Europe/Berlin to the "Run unit and component tests" step env.

startOfDay() uses setHours(0, 0, 0, 0) which applies the local timezone. The CI runner runs in UTC; Berlin midnight dates (+02:00) are 22:00 UTC the previous day, causing bucketByDay to return the wrong bucket.

Verified: TZ=UTC npx vitest run date-buckets.spec.ts → 2 failures. TZ=Europe/Berlin → 7/7 green.

Failure 2 — Testcontainers Docker daemon not found (234 errors)

Added two env vars to the backend-unit-tests job:

Var	Value	Reason
DOCKER_HOST	unix:///var/run/docker.sock	Makes the socket path explicit; Testcontainers finds Docker via env var rather than relying on runner-config propagation
TESTCONTAINERS_RYUK_DISABLED	true	Avoids Ryuk watchdog start failures in nested container environments

runner-config.yaml

Committed the existing (untracked) act_runner config to the repo. Documents the NAS runner configuration required for Docker socket availability. Must be deployed to the runner host alongside the act_runner binary.

Tests

• TZ=UTC date-buckets.spec.ts was red before, is green after CI change ✅
• Testcontainers Docker fix verified by the existing integration test suite (cannot be tested locally without CI environment)

Closes #476 ## What changed ### Failure 1 — `date-buckets.spec.ts` timezone (2 tests) Added `TZ: Europe/Berlin` to the "Run unit and component tests" step env. `startOfDay()` uses `setHours(0, 0, 0, 0)` which applies the **local timezone**. The CI runner runs in UTC; Berlin midnight dates (+02:00) are 22:00 UTC the previous day, causing `bucketByDay` to return the wrong bucket. Verified: `TZ=UTC npx vitest run date-buckets.spec.ts` → 2 failures. `TZ=Europe/Berlin` → 7/7 green. ### Failure 2 — Testcontainers Docker daemon not found (234 errors) Added two env vars to the `backend-unit-tests` job: | Var | Value | Reason | |---|---|---| | `DOCKER_HOST` | `unix:///var/run/docker.sock` | Makes the socket path explicit; Testcontainers finds Docker via env var rather than relying on runner-config propagation | | `TESTCONTAINERS_RYUK_DISABLED` | `true` | Avoids Ryuk watchdog start failures in nested container environments | ### runner-config.yaml Committed the existing (untracked) act_runner config to the repo. Documents the NAS runner configuration required for Docker socket availability. Must be deployed to the runner host alongside the act_runner binary. ## Tests - `TZ=UTC date-buckets.spec.ts` was red before, is green after CI change ✅ - Testcontainers Docker fix verified by the existing integration test suite (cannot be tested locally without CI environment)

marcel referenced this pull request 2026-05-09 16:02:38 +02:00

fix(ci): two persistent CI failures — date-buckets timezone + Testcontainers Docker #476

marcel added 3 commits 2026-05-09 16:03:48 +02:00

fix(ci): add TZ=Europe/Berlin to frontend test step ... ac999066dd

date-buckets.spec.ts midnight tests pass timezone-aware dates (+02:00)
which are 22:00 UTC the prior day; setHours(0,0,0,0) uses local TZ.

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

fix(ci): expose Docker socket env vars for Testcontainers in backend job ... a158048f45

DOCKER_HOST makes the socket explicit rather than relying on runner
config propagation; TESTCONTAINERS_RYUK_DISABLED=true avoids Ryuk
watchdog start failures in nested container environments.

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

ci: track act_runner config with Docker socket mount ... 5512790d5a

Documents the NAS runner configuration needed for Testcontainers.
Must be deployed to the runner host alongside the act_runner binary.

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

marcel force-pushed fix/issue-476-ci-failures from ab7fe81b2a to 5512790d5a 2026-05-09 16:03:48 +02:00 Compare

marcel commented 2026-05-09 16:04:16 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

No application code changed. This is a pure CI config fix — exactly the right scope for a "two unrelated CI failures" bug.

What I checked

TDD discipline: The "red" phase for the TZ fix is documented in the PR description with exact verification steps (TZ=UTC → 2 failures, TZ=Europe/Berlin → 7/7 green). Since the tests already exist in date-buckets.spec.ts, this is the correct TDD pattern for a CI environment fix — you can't write a new failing test when the failing test is already there. ✅

Atomicity: Three commits, three distinct concerns (TZ fix, Docker env vars, runner config tracking). No bundling. ✅

No new production code: ci.yml and runner-config.yaml only. Zero risk of accidental behavior change. ✅

One note (non-blocking): The TZ env is scoped to the "Run unit and component tests" step. If a future test is added in a different step (e.g. a separate --run step) that also uses timezone-sensitive code, it won't pick up the TZ. Applying TZ: Europe/Berlin at the job level instead of the step level would be belt-and-suspenders — but the current scoping matches exactly what's failing, so I won't flag it as a blocker.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** No application code changed. This is a pure CI config fix — exactly the right scope for a "two unrelated CI failures" bug. ### What I checked **TDD discipline:** The "red" phase for the TZ fix is documented in the PR description with exact verification steps (`TZ=UTC → 2 failures`, `TZ=Europe/Berlin → 7/7 green`). Since the tests already exist in `date-buckets.spec.ts`, this is the correct TDD pattern for a CI environment fix — you can't write a new failing test when the failing test is already there. ✅ **Atomicity:** Three commits, three distinct concerns (TZ fix, Docker env vars, runner config tracking). No bundling. ✅ **No new production code:** `ci.yml` and `runner-config.yaml` only. Zero risk of accidental behavior change. ✅ **One note (non-blocking):** The `TZ` env is scoped to the "Run unit and component tests" step. If a future test is added in a different step (e.g. a separate `--run` step) that also uses timezone-sensitive code, it won't pick up the TZ. Applying `TZ: Europe/Berlin` at the job level instead of the step level would be belt-and-suspenders — but the current scoping matches exactly what's failing, so I won't flag it as a blocker.

marcel commented 2026-05-09 16:04:22 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

Two targeted env var additions and committing the runner config. No architectural concerns.

What I checked

Infrastructure-as-code principle: Committing runner-config.yaml to the repository is the right move. A self-hosted runner configuration that lives only on the NAS filesystem is invisible to the development process and gets lost on server reinstall. Tracking it here documents the requirement and makes it reproducible. ✅

Scope: The CI config changes are minimal and surgical. TZ: Europe/Berlin on the test step is the correct fix at the correct layer — the tests encode Berlin-time semantics, CI should match. ✅

No documentation updates required: This PR adds no new services, no new packages, no schema changes, no API routes. The documentation update matrix produces zero required updates. ✅

Note for deployment: The PR description correctly notes that runner-config.yaml "must be deployed to the runner host alongside the act_runner binary." This manual step should be logged somewhere (Gitea wiki, ops runbook) so it isn't forgotten after a future NAS rebuild.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** Two targeted env var additions and committing the runner config. No architectural concerns. ### What I checked **Infrastructure-as-code principle:** Committing `runner-config.yaml` to the repository is the right move. A self-hosted runner configuration that lives only on the NAS filesystem is invisible to the development process and gets lost on server reinstall. Tracking it here documents the requirement and makes it reproducible. ✅ **Scope:** The CI config changes are minimal and surgical. `TZ: Europe/Berlin` on the test step is the correct fix at the correct layer — the tests encode Berlin-time semantics, CI should match. ✅ **No documentation updates required:** This PR adds no new services, no new packages, no schema changes, no API routes. The documentation update matrix produces zero required updates. ✅ **Note for deployment:** The PR description correctly notes that `runner-config.yaml` "must be deployed to the runner host alongside the act_runner binary." This manual step should be logged somewhere (Gitea wiki, ops runbook) so it isn't forgotten after a future NAS rebuild.

marcel commented 2026-05-09 16:04:34 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Security Engineer

Verdict: ⚠️ Approved with concerns

The fixes are correct and the concern is about the Docker socket mount — this is a known trade-off, not a blocker, but it should be documented.

What I checked

DOCKER_HOST: unix:///var/run/docker.sock — This env var tells the JVM process to connect to the host Docker daemon via the Unix socket. It only matters if the socket is actually mounted (which runner-config.yaml ensures). No issue here in isolation. ✅

TESTCONTAINERS_RYUK_DISABLED: "true" — Ryuk is the Testcontainers watchdog that cleans up containers after test runs. Disabling it means orphaned containers can survive if the test process crashes. In CI (ephemeral environments), this is safe — the job container dies and takes its children with it. In a persistent dev environment it would be a concern. ✅ for CI.

Docker socket mount in runner-config.yaml (CWE-269 — Improper Privilege Management):

options: "-v /var/run/docker.sock:/var/run/docker.sock"

Mounting the Docker socket into a container is effectively granting root access to the host, because any process inside the container can spawn privileged containers that mount host paths. For a self-hosted NAS running a private family archive, this risk is acceptable — the runner doesn't accept untrusted code.

Suggestion (non-blocking)

Add a comment to runner-config.yaml documenting the security trade-off:

# SECURITY NOTE: Mounting the Docker socket grants the job container root-equivalent
# access to the host Docker daemon. Acceptable for this private self-hosted runner
# because only trusted code (from this repo's CI) runs here. Do NOT use this config
# on a runner that executes untrusted PRs from external contributors.
options: "-v /var/run/docker.sock:/var/run/docker.sock"

This is especially important if the NAS runner is ever opened to fork PRs.

## 🔐 Nora "NullX" Steiner — Security Engineer **Verdict: ⚠️ Approved with concerns** The fixes are correct and the concern is about the Docker socket mount — this is a known trade-off, not a blocker, but it should be documented. ### What I checked **`DOCKER_HOST: unix:///var/run/docker.sock`** — This env var tells the JVM process to connect to the host Docker daemon via the Unix socket. It only matters if the socket is actually mounted (which `runner-config.yaml` ensures). No issue here in isolation. ✅ **`TESTCONTAINERS_RYUK_DISABLED: "true"`** — Ryuk is the Testcontainers watchdog that cleans up containers after test runs. Disabling it means orphaned containers can survive if the test process crashes. In CI (ephemeral environments), this is safe — the job container dies and takes its children with it. In a persistent dev environment it would be a concern. ✅ for CI. **Docker socket mount in `runner-config.yaml`** (CWE-269 — Improper Privilege Management): ```yaml options: "-v /var/run/docker.sock:/var/run/docker.sock" ``` Mounting the Docker socket into a container is effectively granting root access to the host, because any process inside the container can spawn privileged containers that mount host paths. For a self-hosted NAS running a private family archive, this risk is acceptable — the runner doesn't accept untrusted code. ### Suggestion (non-blocking) Add a comment to `runner-config.yaml` documenting the security trade-off: ```yaml # SECURITY NOTE: Mounting the Docker socket grants the job container root-equivalent # access to the host Docker daemon. Acceptable for this private self-hosted runner # because only trusted code (from this repo's CI) runs here. Do NOT use this config # on a runner that executes untrusted PRs from external contributors. options: "-v /var/run/docker.sock:/var/run/docker.sock" ``` This is especially important if the NAS runner is ever opened to fork PRs.

marcel commented 2026-05-09 16:04:38 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer

Verdict: ✅ Approved

Both fixes are well-specified and the verification approach is correct given the constraints.

What I checked

TZ fix — verified locally: PR description shows exact red/green evidence:

• TZ=UTC vitest run date-buckets.spec.ts → 2 failures ✅ (confirmed in implementation)
• TZ=Europe/Berlin vitest run date-buckets.spec.ts → 7/7 green ✅

This is the appropriate verification method. You cannot write a new failing unit test for "CI runs in UTC" — the existing tests already encode the expected behavior. The fix is environmental. ✅

Testcontainers fix — CI-only: Correct assessment that this cannot be verified locally. The acceptance criterion ("Backend integration tests pass in CI") requires an actual CI run. The approach (DOCKER_HOST + TESTCONTAINERS_RYUK_DISABLED) is the documented standard for Testcontainers in nested container environments. ✅

No test regressions introduced: The changes are additive env vars. No test logic touched. ✅

One process note (non-blocking): After this PR merges, it would be worth tagging the first green CI run in a comment on the issue as verification that the Testcontainers fix works. Issue #476 acceptance criterion 2 can't be fully signed off until a CI run confirms it.

## 🧪 Sara Holt — QA Engineer **Verdict: ✅ Approved** Both fixes are well-specified and the verification approach is correct given the constraints. ### What I checked **TZ fix — verified locally:** PR description shows exact red/green evidence: - `TZ=UTC vitest run date-buckets.spec.ts` → 2 failures ✅ (confirmed in implementation) - `TZ=Europe/Berlin vitest run date-buckets.spec.ts` → 7/7 green ✅ This is the appropriate verification method. You cannot write a new failing unit test for "CI runs in UTC" — the existing tests already encode the expected behavior. The fix is environmental. ✅ **Testcontainers fix — CI-only:** Correct assessment that this cannot be verified locally. The acceptance criterion ("Backend integration tests pass in CI") requires an actual CI run. The approach (`DOCKER_HOST` + `TESTCONTAINERS_RYUK_DISABLED`) is the documented standard for Testcontainers in nested container environments. ✅ **No test regressions introduced:** The changes are additive env vars. No test logic touched. ✅ **One process note (non-blocking):** After this PR merges, it would be worth tagging the first green CI run in a comment on the issue as verification that the Testcontainers fix works. Issue #476 acceptance criterion 2 can't be fully signed off until a CI run confirms it.

marcel commented 2026-05-09 16:04:42 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

No UI, frontend component, or accessibility changes in this diff. This is a CI infrastructure-only fix.

What I checked

• No Svelte components modified ✅
• No Tailwind classes changed ✅
• No ARIA attributes modified ✅
• No user-visible text or layout touched ✅

This PR unblocks CI, which means PRs with accessibility improvements (like #493) can now be verified end-to-end in the pipeline. LGTM from a UX perspective.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** No UI, frontend component, or accessibility changes in this diff. This is a CI infrastructure-only fix. ### What I checked - No Svelte components modified ✅ - No Tailwind classes changed ✅ - No ARIA attributes modified ✅ - No user-visible text or layout touched ✅ This PR unblocks CI, which means PRs with accessibility improvements (like #493) can now be verified end-to-end in the pipeline. LGTM from a UX perspective.

marcel commented 2026-05-09 16:04:51 +02:00

Author

Owner

Copy Link

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

These are exactly the right fixes. Let me walk through each one.

TZ: Europe/Berlin on the test step

Correct and minimal. Scoped to the one step that needs it, not broadcast to all steps. The CI runner image (mcr.microsoft.com/playwright:v1.58.2-noble) is based on Ubuntu Noble which defaults to UTC — setting TZ explicitly at the step level is the clean solution. ✅

DOCKER_HOST + TESTCONTAINERS_RYUK_DISABLED

Both are the documented standard fixes for Testcontainers in a Docker-in-Docker or socket-forwarding environment:

• DOCKER_HOST=unix:///var/run/docker.sock — tells Testcontainers where to connect. Without this, Testcontainers walks a detection chain that may fail in the runner container environment. ✅
• TESTCONTAINERS_RYUK_DISABLED=true — Ryuk tries to run as a privileged container; in environments where Docker permissions are limited, this fails and blocks all tests. Disabling it is safe in ephemeral CI environments. ✅

runner-config.yaml

Good to have this tracked. The config is correct:

• docker_host sets DOCKER_HOST inside job containers (belt-and-suspenders alongside the workflow-level env var) ✅
• valid_volumes whitelists the socket path so the runner allows the bind mount ✅
• options with -v /var/run/docker.sock:/var/run/docker.sock ensures every job container gets the socket ✅

One practical note: The NAS runner needs to be restarted with --config runner-config.yaml (or the equivalent systemd unit updated) after deploying this file. Worth adding to the ops runbook or a README note near the file.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** These are exactly the right fixes. Let me walk through each one. ### `TZ: Europe/Berlin` on the test step Correct and minimal. Scoped to the one step that needs it, not broadcast to all steps. The CI runner image (`mcr.microsoft.com/playwright:v1.58.2-noble`) is based on Ubuntu Noble which defaults to UTC — setting TZ explicitly at the step level is the clean solution. ✅ ### `DOCKER_HOST` + `TESTCONTAINERS_RYUK_DISABLED` Both are the documented standard fixes for Testcontainers in a Docker-in-Docker or socket-forwarding environment: - `DOCKER_HOST=unix:///var/run/docker.sock` — tells Testcontainers where to connect. Without this, Testcontainers walks a detection chain that may fail in the runner container environment. ✅ - `TESTCONTAINERS_RYUK_DISABLED=true` — Ryuk tries to run as a privileged container; in environments where Docker permissions are limited, this fails and blocks all tests. Disabling it is safe in ephemeral CI environments. ✅ ### `runner-config.yaml` Good to have this tracked. The config is correct: - `docker_host` sets `DOCKER_HOST` inside job containers (belt-and-suspenders alongside the workflow-level env var) ✅ - `valid_volumes` whitelists the socket path so the runner allows the bind mount ✅ - `options` with `-v /var/run/docker.sock:/var/run/docker.sock` ensures every job container gets the socket ✅ **One practical note:** The NAS runner needs to be restarted with `--config runner-config.yaml` (or the equivalent systemd unit updated) after deploying this file. Worth adding to the ops runbook or a README note near the file.

marcel commented 2026-05-09 16:04:56 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Two acceptance criteria, two targeted fixes. Clean traceability.

Requirements traceability

Acceptance Criterion	Fix	Verification
date-buckets.spec.ts both failing tests green in CI	TZ: Europe/Berlin on test step	✅ Locally verified (TZ=UTC red → TZ=Europe/Berlin green)
Backend integration tests pass (Testcontainers can start postgres)	DOCKER_HOST + TESTCONTAINERS_RYUK_DISABLED on backend job	⏳ Requires CI run to confirm
No other tests regress	Additive-only env vars, no code changes	✅ By construction

Scope assessment

The PR is correctly scoped to the issue. The runner-config.yaml addition is infrastructure documentation that directly supports acceptance criterion 2 — it provides the Docker socket availability that the env vars depend on. Appropriate scope expansion. ✅

One open item

Acceptance criterion 2 ("Backend integration tests pass in CI") cannot be fully verified without a CI run. This is expected and documented in the PR. The issue should be left open until the first green CI run confirms it, then closed.

No requirements gaps or ambiguities identified.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** Two acceptance criteria, two targeted fixes. Clean traceability. ### Requirements traceability | Acceptance Criterion | Fix | Verification | |---|---|---| | `date-buckets.spec.ts` both failing tests green in CI | `TZ: Europe/Berlin` on test step | ✅ Locally verified (TZ=UTC red → TZ=Europe/Berlin green) | | Backend integration tests pass (Testcontainers can start postgres) | `DOCKER_HOST` + `TESTCONTAINERS_RYUK_DISABLED` on backend job | ⏳ Requires CI run to confirm | | No other tests regress | Additive-only env vars, no code changes | ✅ By construction | ### Scope assessment The PR is correctly scoped to the issue. The `runner-config.yaml` addition is infrastructure documentation that directly supports acceptance criterion 2 — it provides the Docker socket availability that the env vars depend on. Appropriate scope expansion. ✅ ### One open item Acceptance criterion 2 ("Backend integration tests pass in CI") cannot be fully verified without a CI run. This is expected and documented in the PR. The issue should be left open until the first green CI run confirms it, then closed. No requirements gaps or ambiguities identified.

marcel added 1 commit 2026-05-09 16:05:55 +02:00

ci: document Docker socket security trade-off in runner config ... 6074480482

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

marcel commented 2026-05-09 16:06:30 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Clean, minimal CI fix. Nothing to flag from a code quality or TDD perspective.

What I checked

• 20 additions, 0 deletions, 2 files — entirely in CI config. No production code touched. ✅
• Three atomic commits (TZ fix, Docker env vars, runner config). Each does one thing. ✅
• TZ: Europe/Berlin scoped to the test step. Correct and precise. ✅
• Previous round's non-blocking note about job-level TZ scoping stands, but step-level is sufficient for the current failure. ✅
• Security note added to runner-config.yaml as suggested by Nora. ✅

LGTM.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Clean, minimal CI fix. Nothing to flag from a code quality or TDD perspective. ### What I checked - 20 additions, 0 deletions, 2 files — entirely in CI config. No production code touched. ✅ - Three atomic commits (TZ fix, Docker env vars, runner config). Each does one thing. ✅ - `TZ: Europe/Berlin` scoped to the test step. Correct and precise. ✅ - Previous round's non-blocking note about job-level TZ scoping stands, but step-level is sufficient for the current failure. ✅ - Security note added to runner-config.yaml as suggested by Nora. ✅ LGTM.

marcel commented 2026-05-09 16:06:33 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

No change from my previous review. The PR remains correctly scoped to CI infrastructure — zero application architecture impact.

runner-config.yaml is now tracked in the repo with appropriate comments explaining both the security trade-off and the networking behavior. Infrastructure-as-code done right. ✅

No documentation update requirements triggered. Ready to merge.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** No change from my previous review. The PR remains correctly scoped to CI infrastructure — zero application architecture impact. `runner-config.yaml` is now tracked in the repo with appropriate comments explaining both the security trade-off and the networking behavior. Infrastructure-as-code done right. ✅ No documentation update requirements triggered. Ready to merge.

marcel commented 2026-05-09 16:06:36 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Security Engineer

Verdict: ✅ Approved

My previous concern has been addressed. The Docker socket security trade-off is now explicitly documented in runner-config.yaml:

# SECURITY: Mounting the Docker socket grants job containers root-equivalent
# access to the host Docker daemon. Acceptable here because only trusted code
# from this private repo runs on this runner. Do NOT use on a runner that
# accepts untrusted PRs from external contributors.

This is exactly what I asked for — the threat model is stated, the risk acceptance is justified, and the guard condition against misuse is explicit. A future operator reading this config will understand why it's configured this way before copying it elsewhere. ✅

No remaining security concerns.

## 🔐 Nora "NullX" Steiner — Security Engineer **Verdict: ✅ Approved** My previous concern has been addressed. The Docker socket security trade-off is now explicitly documented in `runner-config.yaml`: ```yaml # SECURITY: Mounting the Docker socket grants job containers root-equivalent # access to the host Docker daemon. Acceptable here because only trusted code # from this private repo runs on this runner. Do NOT use on a runner that # accepts untrusted PRs from external contributors. ``` This is exactly what I asked for — the threat model is stated, the risk acceptance is justified, and the guard condition against misuse is explicit. A future operator reading this config will understand why it's configured this way before copying it elsewhere. ✅ No remaining security concerns.

marcel commented 2026-05-09 16:06:39 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer

Verdict: ✅ Approved

No change from my previous assessment. Both fixes are correct, verification evidence is documented in the PR description, and the security note addition is a non-functional improvement that adds no test risk.

My process note from the last round still stands: after merge, confirm the first green CI run on this branch and post it as a comment closing issue #476 acceptance criterion 2. ✅

## 🧪 Sara Holt — QA Engineer **Verdict: ✅ Approved** No change from my previous assessment. Both fixes are correct, verification evidence is documented in the PR description, and the security note addition is a non-functional improvement that adds no test risk. My process note from the last round still stands: after merge, confirm the first green CI run on this branch and post it as a comment closing issue #476 acceptance criterion 2. ✅

marcel commented 2026-05-09 16:06:42 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

CI infrastructure only. No UI, accessibility, or brand changes. LGTM.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** CI infrastructure only. No UI, accessibility, or brand changes. LGTM.

marcel commented 2026-05-09 16:06:44 +02:00

Author

Owner

Copy Link

🚀 Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

The security comment is a welcome addition. The full runner-config.yaml is now self-documenting:

• docker_host — explains the DOCKER_HOST propagation mechanism ✅
• valid_volumes — explains the whitelist purpose ✅
• options — now has the security trade-off documented ✅
• force_pull: false — explains the networking decision ✅

My deployment note from last round stands: the NAS runner needs --config runner-config.yaml in its systemd unit. That's an ops task outside the PR scope — not a blocker.

Ready to merge.

## 🚀 Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** The security comment is a welcome addition. The full `runner-config.yaml` is now self-documenting: - `docker_host` — explains the DOCKER_HOST propagation mechanism ✅ - `valid_volumes` — explains the whitelist purpose ✅ - `options` — now has the security trade-off documented ✅ - `force_pull: false` — explains the networking decision ✅ My deployment note from last round stands: the NAS runner needs `--config runner-config.yaml` in its systemd unit. That's an ops task outside the PR scope — not a blocker. Ready to merge.

marcel commented 2026-05-09 16:06:48 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

All acceptance criteria covered, Nora's concern resolved. The security documentation addition is a net positive with no requirements implications.

Acceptance Criterion	Status
date-buckets.spec.ts both failing tests green in CI	✅ Fix in place, locally verified
Backend integration tests pass (Testcontainers can start postgres)	⏳ Requires first CI run post-merge
No other tests regress	✅ Additive-only changes

No remaining open requirements questions.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** All acceptance criteria covered, Nora's concern resolved. The security documentation addition is a net positive with no requirements implications. | Acceptance Criterion | Status | |---|---| | `date-buckets.spec.ts` both failing tests green in CI | ✅ Fix in place, locally verified | | Backend integration tests pass (Testcontainers can start postgres) | ⏳ Requires first CI run post-merge | | No other tests regress | ✅ Additive-only changes | No remaining open requirements questions.

marcel merged commit 6074480482 into main 2026-05-09 16:08:49 +02:00

marcel deleted branch fix/issue-476-ci-failures 2026-05-09 16:08:53 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#494