cleanup(legibility): remove dead code identified by audits #412

New Issue

marcel · 2026-05-04T16:17:02+02:00

marcel commented

2026-05-04 16:17:02 +02:00

Context

Part of Epic #411 — Cleanup. This is CLEANUP-1: act on the C10.1 findings from the audit reports — orphan classes, never-imported modules, .old / .backup directories, half-finished features (if (false) branches, commented-out blocks), unjustified premature abstractions.

Per the Legibility Rubric, this addresses C10.1, C10.2, C10.4 (Major).

Inputs

§7 (Dead/suspicious code register) of every audit report under docs/audits/
- audit-frontend-report.md (#388)
- audit-backend-report.md (#389)
- audit-ocr-report.md (#390)
- audit-db-report.md (#391)
- audit-rest-report.md (#392)

Method

Per stack:

Walk the §7 register
For each item, decide:
- Delete — confirmed dead, no consumer, no future use
- Annotate — kept for a reason; add a why-comment OR a // TODO(legibility): justify or remove with an owner+date
- Defer — too risky to delete in this issue; open a follow-up issue with a P2 label
Verify each delete with a build + test pass
Use one PR per stack (so REFACTOR-1 reviewers can re-verify)

Specific known cleanup candidates (from requirements-phase research)

These were noted but not exhaustively analyzed; the audits will confirm:

frontend/.svelte-kit.old/ — likely safe to delete after build verified
frontend/test-results.locked/ — probably stale test output; investigate
backend/CLAUDE.md "Layering Rules (strictly enforced)" section — already obsolete after REFACTOR-1; handled by DOC-7 (#401), don't duplicate work here
scripts/schema.sql — note: per scripts/CLAUDE.md it's a snapshot, not source-of-truth; don't delete unless docs are updated

Acceptance criteria

Every C10.1 finding from every audit report has been classified (delete / annotate / defer)
Per-stack PR opened (backend + frontend) and merged
Full backend test suite green (./mvnw test)
Full frontend test suite + e2e green (npm run test && npx playwright test)
Closing comment lists how many items were deleted vs annotated vs deferred per stack

Dependency

Hard dependency on Epic 1 audit reports (need §7 registers).

Soft dependency on Epic 4 refactor — easier to verify "no consumer" after the move.

Definition of Done

PRs merged; closing comment summarizes counts.

## Context Part of **Epic #411** — Cleanup. This is **CLEANUP-1**: act on the C10.1 findings from the audit reports — orphan classes, never-imported modules, `.old` / `.backup` directories, half-finished features (`if (false)` branches, commented-out blocks), unjustified premature abstractions. Per the Legibility Rubric, this addresses **C10.1, C10.2, C10.4 (Major)**. ## Inputs - §7 (Dead/suspicious code register) of every audit report under `docs/audits/` - `audit-frontend-report.md` (#388) - `audit-backend-report.md` (#389) - `audit-ocr-report.md` (#390) - `audit-db-report.md` (#391) - `audit-rest-report.md` (#392) ## Method Per stack: 1. Walk the §7 register 2. For each item, decide: - **Delete** — confirmed dead, no consumer, no future use - **Annotate** — kept for a reason; add a why-comment OR a `// TODO(legibility): justify or remove` with an owner+date - **Defer** — too risky to delete in this issue; open a follow-up issue with a P2 label 3. Verify each delete with a build + test pass 4. Use one PR per stack (so REFACTOR-1 reviewers can re-verify) ## Specific known cleanup candidates (from requirements-phase research) These were noted but not exhaustively analyzed; the audits will confirm: - `frontend/.svelte-kit.old/` — likely safe to delete after build verified - `frontend/test-results.locked/` — probably stale test output; investigate - `backend/CLAUDE.md` "Layering Rules (strictly enforced)" section — already obsolete after REFACTOR-1; handled by DOC-7 (#401), don't duplicate work here - `scripts/schema.sql` — note: per `scripts/CLAUDE.md` it's a snapshot, not source-of-truth; don't delete unless docs are updated ## Acceptance criteria - [ ] Every C10.1 finding from every audit report has been classified (delete / annotate / defer) - [ ] Per-stack PR opened (backend + frontend) and merged - [ ] Full backend test suite green (`./mvnw test`) - [ ] Full frontend test suite + e2e green (`npm run test && npx playwright test`) - [ ] Closing comment lists how many items were deleted vs annotated vs deferred per stack ## Dependency **Hard dependency** on Epic 1 audit reports (need §7 registers). Soft dependency on Epic 4 refactor — easier to verify "no consumer" after the move. ## Definition of Done PRs merged; closing comment summarizes counts.

marcel added this to the (deleted) milestone 2026-05-04 16:17:02 +02:00

marcel added the P2-medium cleanup legibility labels 2026-05-04 16:18:36 +02:00

marcel referenced this issue

2026-05-04 21:05:02 +02:00

epic(legibility): polish — remove smells surfaced by audits #411

marcel commented

2026-05-04 21:05:03 +02:00

🏗️ Markus Keller — Senior Application Architect

Observations

The issue correctly frames the "annotate vs. delete vs. defer" trichotomy. That's the right decision model — the C10 rubric is about legibility, and not every dead-looking artifact should be axed.
frontend/.svelte-kit.old/ and frontend/test-results.locked/ are untracked in git and already covered by .gitignore patterns (/.svelte-kit-backup, **/test-results/). The .svelte-kit.old dir contains a single stale type stub for a .stammbaum-stale route that no longer exists in the active route tree — this is clearly safe to delete. The test-results.locked/ directory is 20KB of Playwright artifacts and .last-run.json — delete without ceremony.
The "Layering Rules (strictly enforced)" section in backend/CLAUDE.md is explicitly called out for DOC-7 (#401). The issue correctly flags it as "don't duplicate work here." Respect that boundary — this cleanup should not touch content owned by DOC-7.
scripts/schema.sql is documented in scripts/CLAUDE.md as a snapshot, not source-of-truth. The right call here is "annotate" (the CLAUDE.md already does this) — do not delete unless the note is updated to explain its purpose to a new reader.
The "per stack PR" instruction is sound. Backend and frontend diffs are reviewed by different mental models. One PR per stack keeps the review focused.
The issue has a hard dependency on the audit reports (§7 registers from #388–#392) but those reports live in docs/audits/ — that directory does not currently exist in the codebase. This cleanup work cannot meaningfully start until the audit reports are produced and committed. The issue's own "Dependency" section acknowledges this, but the acceptance criteria don't reflect it.

Recommendations

Before opening PRs, verify the audit report directory exists (docs/audits/*.md). If it doesn't, this issue is blocked and should be labeled blocked.
For each "defer" decision: open the follow-up issue at the time of deferral, not after. A deferred item with no ticket is effectively deleted from the team's memory.
The .svelte-kit.old/ directory's presence is a signal that something was manually renamed rather than deleted. Check if .gitignore patterns should be tightened to prevent recurrence: add **/.svelte-kit.old/ and ensure test-results.locked/ is also covered.
Do not create a docs/adr/ entry for this cleanup — it is execution work, not an architectural decision. Reserve ADRs for decisions with tradeoffs.

Open Decisions

Scope of "dead code" in scripts: scripts/schema.sql is documented as a snapshot. But scripts/large-data.sql and scripts/generate_data.py are also present and undocumented in scripts/CLAUDE.md. Are those in scope for this issue, or deferred to a follow-up? The audit reports (§7) should make this explicit — but if they don't, a human call is needed before the PR.

## 🏗️ Markus Keller — Senior Application Architect ### Observations - The issue correctly frames the "annotate vs. delete vs. defer" trichotomy. That's the right decision model — the C10 rubric is about legibility, and not every dead-looking artifact should be axed. - `frontend/.svelte-kit.old/` and `frontend/test-results.locked/` are untracked in git and already covered by `.gitignore` patterns (`/.svelte-kit-backup`, `**/test-results/`). The `.svelte-kit.old` dir contains a single stale type stub for a `.stammbaum-stale` route that no longer exists in the active route tree — this is clearly safe to delete. The `test-results.locked/` directory is 20KB of Playwright artifacts and `.last-run.json` — delete without ceremony. - The "Layering Rules (strictly enforced)" section in `backend/CLAUDE.md` is explicitly called out for DOC-7 (#401). The issue correctly flags it as "don't duplicate work here." Respect that boundary — this cleanup should not touch content owned by DOC-7. - `scripts/schema.sql` is documented in `scripts/CLAUDE.md` as a snapshot, not source-of-truth. The right call here is "annotate" (the CLAUDE.md already does this) — do not delete unless the note is updated to explain its purpose to a new reader. - The "per stack PR" instruction is sound. Backend and frontend diffs are reviewed by different mental models. One PR per stack keeps the review focused. - The issue has a hard dependency on the audit reports (§7 registers from #388–#392) but those reports live in `docs/audits/` — **that directory does not currently exist in the codebase**. This cleanup work cannot meaningfully start until the audit reports are produced and committed. The issue's own "Dependency" section acknowledges this, but the acceptance criteria don't reflect it. ### Recommendations - Before opening PRs, verify the audit report directory exists (`docs/audits/*.md`). If it doesn't, this issue is blocked and should be labeled `blocked`. - For each "defer" decision: open the follow-up issue at the time of deferral, not after. A deferred item with no ticket is effectively deleted from the team's memory. - The `.svelte-kit.old/` directory's presence is a signal that something was manually renamed rather than deleted. Check if `.gitignore` patterns should be tightened to prevent recurrence: add `**/.svelte-kit.old/` and ensure `test-results.locked/` is also covered. - Do not create a `docs/adr/` entry for this cleanup — it is execution work, not an architectural decision. Reserve ADRs for decisions with tradeoffs. ### Open Decisions - **Scope of "dead code" in scripts**: `scripts/schema.sql` is documented as a snapshot. But `scripts/large-data.sql` and `scripts/generate_data.py` are also present and undocumented in `scripts/CLAUDE.md`. Are those in scope for this issue, or deferred to a follow-up? The audit reports (§7) should make this explicit — but if they don't, a human call is needed before the PR.

marcel commented

2026-05-04 21:05:32 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

frontend/.svelte-kit.old/ is untracked, 24KB, and contains only a stale type stub for .stammbaum-stale — a route variant that doesn't match any live route in src/routes/stammbaum/. This is unambiguously dead. Delete it.
frontend/test-results.locked/ is also untracked, covered by .gitignore, and contains only Playwright trace artifacts and a .last-run.json. This should never have been committed or left lying around. Delete it.
The frontend has 474 source files. The §7 audit registers (not yet present in docs/audits/) are the proper starting point — don't guess at dead imports before reading them.
I checked frontend/src/lib/components/chronik/ChronikRow.svelte — it contains a TODO about the backend not yet exposing a comment body preview. That's a "defer" candidate, not delete: it's a real gap note pointing to a missing backend feature. The right move is annotate with a // TODO(legibility): justify or remove — tracked in <new-issue-url> if no issue exists yet.
frontend/src/lib/errors.ts and frontend/src/lib/utils/hoverCardPosition.ts contain /* */ block comment patterns, but these are not dead code — they're legitimate documentation. Grep for if (false) specifically; nothing came up in the live source tree.
The acceptance criteria say "full frontend test suite + e2e green." Good. For deletions of any .ts or .svelte file, run npm run check (svelte-check) as the first signal — it will catch any import that was relying on the deleted file.

Recommendations

After each delete, run npm run check immediately. Don't accumulate deletes and then check — failing fast is faster overall.
For ChronikRow.svelte's TODO: look up whether there is already a backend issue for comment preview. If not, open one before annotating with // TODO(legibility): ... — the annotation should reference a real ticket, not a vague future intent.
frontend/src/lib/generated/api.ts is auto-generated. Never annotate or modify it directly — regenerate it after any backend cleanup. This file should be regenerated at the end of the backend PR, not the frontend PR.
Use git rm not rm when removing tracked files so the deletion shows up cleanly in the PR diff. For untracked files like .svelte-kit.old/, plain rm -rf is fine.

Open Decisions (omit this section entirely if none)

None — the method is clear: audit §7 registers → delete/annotate/defer, run check, open PR per stack.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - `frontend/.svelte-kit.old/` is untracked, 24KB, and contains only a stale type stub for `.stammbaum-stale` — a route variant that doesn't match any live route in `src/routes/stammbaum/`. This is unambiguously dead. Delete it. - `frontend/test-results.locked/` is also untracked, covered by `.gitignore`, and contains only Playwright trace artifacts and a `.last-run.json`. This should never have been committed or left lying around. Delete it. - The frontend has 474 source files. The §7 audit registers (not yet present in `docs/audits/`) are the proper starting point — don't guess at dead imports before reading them. - I checked `frontend/src/lib/components/chronik/ChronikRow.svelte` — it contains a `TODO` about the backend not yet exposing a comment body preview. That's a "defer" candidate, not delete: it's a real gap note pointing to a missing backend feature. The right move is annotate with a `// TODO(legibility): justify or remove — tracked in <new-issue-url>` if no issue exists yet. - `frontend/src/lib/errors.ts` and `frontend/src/lib/utils/hoverCardPosition.ts` contain `/* */` block comment patterns, but these are not dead code — they're legitimate documentation. Grep for `if (false)` specifically; nothing came up in the live source tree. - The acceptance criteria say "full frontend test suite + e2e green." Good. For deletions of any `.ts` or `.svelte` file, run `npm run check` (svelte-check) as the first signal — it will catch any import that was relying on the deleted file. ### Recommendations - After each delete, run `npm run check` immediately. Don't accumulate deletes and then check — failing fast is faster overall. - For `ChronikRow.svelte`'s TODO: look up whether there is already a backend issue for comment preview. If not, open one before annotating with `// TODO(legibility): ...` — the annotation should reference a real ticket, not a vague future intent. - `frontend/src/lib/generated/api.ts` is auto-generated. Never annotate or modify it directly — regenerate it after any backend cleanup. This file should be regenerated at the end of the backend PR, not the frontend PR. - Use `git rm` not `rm` when removing tracked files so the deletion shows up cleanly in the PR diff. For untracked files like `.svelte-kit.old/`, plain `rm -rf` is fine. ### Open Decisions _(omit this section entirely if none)_ None — the method is clear: audit §7 registers → delete/annotate/defer, run check, open PR per stack.

marcel commented

2026-05-04 21:05:57 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

Dead code removal is a security-positive operation. Commented-out code, unreachable branches, and disabled endpoints reduce the attack surface, eliminate confusion about what is actually active, and make security audits faster. I'm in favor of this issue.
The specific risk I flag for dead code cleanup is accidental deletion of security controls. Some code looks dead but is actually a defense in depth layer: a validation that seems redundant, a null-check that "never triggers," a permission check that "should have been caught earlier." These are the most dangerous things to delete without understanding.
I checked the backend/src/main/java production code for @Deprecated and if (false) patterns — none found. The current codebase is clean on the most obvious signals.
The DocumentVersionService.java has @SuppressWarnings("unchecked") annotations — these are around deserialization casts for version diffing. Not dead code, but worth an eye during the audit: suppressed warnings can mask type-safety issues. The audit register should flag these if it hasn't already.
frontend/test-results.locked/ contains Playwright traces and auth state files. Verify that no real credentials (session tokens, auth cookies) are embedded in the trace artifacts before deleting — trace ZIPs can contain captured network requests including auth headers. This is the trace.zip file in e2e/auth.setup.ts-authenticate-setup/. Delete it, but know what you're deleting.

Recommendations

For every security-adjacent class or method flagged as dead code in the §7 register: require a comment from the implementer (or the git log) explaining why it was added before deleting. If the intent is unclear, annotate with // TODO(legibility): justify or remove rather than deleting.
Specifically check SecurityConfig.java and PermissionAspect.java against the §7 register. Neither should appear there, but if they do, flag as "defer" and open a dedicated security review issue — never treat security infrastructure as normal cleanup.
The scripts/clean-e2e-data.sh script resets test state — verify it's not relying on any test user accounts or seeded auth data that might be eliminated by this cleanup.

Open Decisions

None — security review of dead code is clear: read before deleting, annotate when unsure, never delete security controls without explicit human confirmation.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - Dead code removal is a security-positive operation. Commented-out code, unreachable branches, and disabled endpoints reduce the attack surface, eliminate confusion about what is actually active, and make security audits faster. I'm in favor of this issue. - The specific risk I flag for dead code cleanup is **accidental deletion of security controls**. Some code looks dead but is actually a defense in depth layer: a validation that seems redundant, a null-check that "never triggers," a permission check that "should have been caught earlier." These are the most dangerous things to delete without understanding. - I checked the `backend/src/main/java` production code for `@Deprecated` and `if (false)` patterns — none found. The current codebase is clean on the most obvious signals. - The `DocumentVersionService.java` has `@SuppressWarnings("unchecked")` annotations — these are around deserialization casts for version diffing. Not dead code, but worth an eye during the audit: suppressed warnings can mask type-safety issues. The audit register should flag these if it hasn't already. - `frontend/test-results.locked/` contains Playwright traces and auth state files. Verify that no real credentials (session tokens, auth cookies) are embedded in the trace artifacts before deleting — trace ZIPs can contain captured network requests including auth headers. This is the `trace.zip` file in `e2e/auth.setup.ts-authenticate-setup/`. Delete it, but know what you're deleting. ### Recommendations - For every security-adjacent class or method flagged as dead code in the §7 register: require a comment from the implementer (or the git log) explaining why it was added before deleting. If the intent is unclear, annotate with `// TODO(legibility): justify or remove` rather than deleting. - Specifically check `SecurityConfig.java` and `PermissionAspect.java` against the §7 register. Neither should appear there, but if they do, flag as "defer" and open a dedicated security review issue — never treat security infrastructure as normal cleanup. - The `scripts/clean-e2e-data.sh` script resets test state — verify it's not relying on any test user accounts or seeded auth data that might be eliminated by this cleanup. ### Open Decisions None — security review of dead code is clear: read before deleting, annotate when unsure, never delete security controls without explicit human confirmation.

marcel commented

2026-05-04 21:06:25 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The acceptance criteria include "full backend test suite green" and "full frontend test suite + e2e green." This is exactly right — dead code deletion should never break a passing test suite, and if it does, that's a signal the "dead" code was actually live.
The issue says frontend/test-results.locked/ should be investigated. I checked: it's an untracked directory containing Playwright artifacts (.last-run.json, trace.zip). It's not a stale test directory in the source tree — it's runtime output left over from a previous test run. It's already covered by the root .gitignore (**/test-results/). This is housekeeping, not a test risk.
The biggest test risk for this issue is inadvertently deleting a module that a test imports directly. With 474 frontend source files, there may be utility modules or store helpers that are only imported by test files (not by production routes). npm run check does not catch test-only imports — only npm run test will.
The acceptance criteria do not include: "zero new @Disabled tests introduced." This cleanup should not disable any existing test to make things pass — if a delete breaks a test, either the delete was wrong or the test was covering dead paths and should itself be deleted with explanation.

Recommendations

Run the full test suite (unit + integration + e2e) before making any deletions and capture the baseline. Compare the post-cleanup run to the baseline to confirm nothing regressed. The acceptance criteria say "green" but don't say "same count of passing tests" — the closing comment should report both.
For the backend: ./mvnw test output includes a test count summary. The closing comment should include before/after counts per stack to make it obvious no tests were silently dropped.
@Disabled tests found during the audit should be classified separately: if the test is disabled because the feature is dead, delete both the test and the code together in the same commit. Don't leave orphaned disabled tests pointing at deleted functionality.
The "defer" path in the method should produce a follow-up issue before the PR is merged, not after. A deferred item with no ticket is an unmaintained annotation — the acceptance criteria should require this.

Open Decisions

Handling tests that cover deleted code: if a unit test exercises a method that gets deleted, the test should be deleted too. But the issue's acceptance criteria only say "test suite green" — they don't say "no orphaned tests." Should the closing comment explicitly list tests that were deleted alongside deleted production code? I'd recommend yes, to make the cleanup auditable.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - The acceptance criteria include "full backend test suite green" and "full frontend test suite + e2e green." This is exactly right — dead code deletion should never break a passing test suite, and if it does, that's a signal the "dead" code was actually live. - The issue says `frontend/test-results.locked/` should be investigated. I checked: it's an untracked directory containing Playwright artifacts (`.last-run.json`, `trace.zip`). It's not a stale test directory in the source tree — it's runtime output left over from a previous test run. It's already covered by the root `.gitignore` (`**/test-results/`). This is housekeeping, not a test risk. - The biggest test risk for this issue is **inadvertently deleting a module that a test imports directly**. With 474 frontend source files, there may be utility modules or store helpers that are only imported by test files (not by production routes). `npm run check` does not catch test-only imports — only `npm run test` will. - The acceptance criteria do not include: "zero new `@Disabled` tests introduced." This cleanup should not disable any existing test to make things pass — if a delete breaks a test, either the delete was wrong or the test was covering dead paths and should itself be deleted with explanation. ### Recommendations - Run the full test suite (unit + integration + e2e) **before** making any deletions and capture the baseline. Compare the post-cleanup run to the baseline to confirm nothing regressed. The acceptance criteria say "green" but don't say "same count of passing tests" — the closing comment should report both. - For the backend: `./mvnw test` output includes a test count summary. The closing comment should include before/after counts per stack to make it obvious no tests were silently dropped. - `@Disabled` tests found during the audit should be classified separately: if the test is disabled because the feature is dead, delete both the test and the code together in the same commit. Don't leave orphaned disabled tests pointing at deleted functionality. - The "defer" path in the method should produce a follow-up issue **before** the PR is merged, not after. A deferred item with no ticket is an unmaintained annotation — the acceptance criteria should require this. ### Open Decisions - **Handling tests that cover deleted code**: if a unit test exercises a method that gets deleted, the test should be deleted too. But the issue's acceptance criteria only say "test suite green" — they don't say "no orphaned tests." Should the closing comment explicitly list tests that were deleted alongside deleted production code? I'd recommend yes, to make the cleanup auditable.

marcel referenced this issue

2026-05-04 21:06:27 +02:00

epic(legibility): polish — remove smells surfaced by audits #411

marcel commented

2026-05-04 21:06:45 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

This is infrastructure/code cleanup, not a UI change. From a UX and accessibility perspective, I'm net positive on this issue: dead code creates confusion about which UI states are actually reachable, and commented-out UI branches are a maintenance trap.
The .svelte-kit.old/types stale type for .stammbaum-stale is not a UX concern per se, but it's worth noting: the active stammbaum route (src/routes/stammbaum/+page.svelte) is live and referenced in AppNav.svelte. The stale .stammbaum-stale type stub appears to be an artifact of a route rename. Safe to delete.
ChronikRow.svelte has a TODO about a missing backend feature (comment body preview). That TODO is user-visible debt — without the preview, the Chronik row shows an incomplete state. This is a UX gap, not just dead code. When annotating it with // TODO(legibility): justify or remove, the annotation should also reference the UX consequence, not just the backend gap.
Any commented-out UI code in .svelte files that was for an interaction pattern (e.g., a hover state, a loading skeleton, a modal trigger) should be deleted if the pattern no longer exists in the active UI — not annotated. Annotated commented-out markup is harder to read than absent markup.

Recommendations

When the §7 register contains frontend items, check whether the "dead" code is actually a non-obvious UI state (empty state, error state, loading state). These are easy to delete by mistake because they only appear in edge cases during review.
Do not remove aria-* attributes, role= attributes, or focus management code without verifying the interaction still works for keyboard users. These can look like "unused" markup at a glance.
The closing comment should explicitly call out any UI component that was deleted, not just utility modules — so the UX layer can be verified visually during review.

No open decisions from my angle — this is cleanup work with no new UI surfaces involved.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations - This is infrastructure/code cleanup, not a UI change. From a UX and accessibility perspective, I'm net positive on this issue: dead code creates confusion about which UI states are actually reachable, and commented-out UI branches are a maintenance trap. - The `.svelte-kit.old/types` stale type for `.stammbaum-stale` is not a UX concern per se, but it's worth noting: the active `stammbaum` route (`src/routes/stammbaum/+page.svelte`) is live and referenced in `AppNav.svelte`. The stale `.stammbaum-stale` type stub appears to be an artifact of a route rename. Safe to delete. - `ChronikRow.svelte` has a TODO about a missing backend feature (comment body preview). That TODO is user-visible debt — without the preview, the Chronik row shows an incomplete state. This is a UX gap, not just dead code. When annotating it with `// TODO(legibility): justify or remove`, the annotation should also reference the UX consequence, not just the backend gap. - Any commented-out UI code in `.svelte` files that was for an interaction pattern (e.g., a hover state, a loading skeleton, a modal trigger) should be **deleted** if the pattern no longer exists in the active UI — not annotated. Annotated commented-out markup is harder to read than absent markup. ### Recommendations - When the §7 register contains frontend items, check whether the "dead" code is actually a non-obvious UI state (empty state, error state, loading state). These are easy to delete by mistake because they only appear in edge cases during review. - Do not remove `aria-*` attributes, `role=` attributes, or focus management code without verifying the interaction still works for keyboard users. These can look like "unused" markup at a glance. - The closing comment should explicitly call out any UI component that was deleted, not just utility modules — so the UX layer can be verified visually during review. No open decisions from my angle — this is cleanup work with no new UI surfaces involved.

marcel commented

2026-05-04 21:07:07 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

Dead code cleanup is low-risk from an infrastructure perspective, provided the PRs are tested with the full CI pipeline (not just local builds). The acceptance criteria correctly require both the backend test suite and the full frontend + e2e suite.
The docs/audits/ directory does not currently exist on disk. The issue depends on audit reports that haven't been committed yet. The CI pipeline can't validate against non-existent inputs. This is the critical blocker: until the audit reports land in docs/audits/, this issue should be in a blocked state.
frontend/.svelte-kit.old/ and frontend/test-results.locked/ are both untracked and not in .gitignore at the root level (though **/test-results/ is covered). The frontend/.svelte-kit.old/ pattern is NOT in .gitignore — it will reappear as an untracked item unless the ignore rule is added. Recommend adding **/.svelte-kit.old/ to frontend/.gitignore when deleting the directory.
The "per stack PR" instruction is correct from a CI standpoint: backend and frontend jobs are independent pipelines. A single PR touching both stacks means backend test failures block frontend review and vice versa. Keep them separate.
The ocr-service/CLAUDE.md exists and the issue references audit-ocr-report.md (#390). The OCR service tests are Python/pytest. The acceptance criteria mention only backend and frontend test suites — OCR service tests should also be in the acceptance criteria if the OCR §7 register produces any deletions.

Recommendations

Before starting implementation: verify docs/audits/ exists and contains all five reports. If not, label this issue blocked and link to the blocking audit issues (#388–#392).
When deleting frontend/.svelte-kit.old/, add **/.svelte-kit.old/ to frontend/.gitignore in the same commit. Otherwise the next SvelteKit upgrade cycle may recreate it.
The closing comment format requested by the issue ("how many items were deleted vs annotated vs deferred per stack") doubles as a useful deployment audit trail. Keep it.
If the OCR service has §7 findings, open a third PR (OCR service stack) rather than bundling it into backend or frontend.

Open Decisions

OCR service test gate: the acceptance criteria say "full backend test suite" and "full frontend test suite + e2e" but don't mention ocr-service pytest. If the OCR §7 register has deletions, is a passing OCR test run also required before merge? I'd say yes — but the issue doesn't state it. Clarify before implementation.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - Dead code cleanup is low-risk from an infrastructure perspective, provided the PRs are tested with the full CI pipeline (not just local builds). The acceptance criteria correctly require both the backend test suite and the full frontend + e2e suite. - The `docs/audits/` directory does not currently exist on disk. The issue depends on audit reports that haven't been committed yet. The CI pipeline can't validate against non-existent inputs. This is the critical blocker: until the audit reports land in `docs/audits/`, this issue should be in a `blocked` state. - `frontend/.svelte-kit.old/` and `frontend/test-results.locked/` are both untracked and not in `.gitignore` at the root level (though `**/test-results/` is covered). The `frontend/.svelte-kit.old/` pattern is NOT in `.gitignore` — it will reappear as an untracked item unless the ignore rule is added. Recommend adding `**/.svelte-kit.old/` to `frontend/.gitignore` when deleting the directory. - The "per stack PR" instruction is correct from a CI standpoint: backend and frontend jobs are independent pipelines. A single PR touching both stacks means backend test failures block frontend review and vice versa. Keep them separate. - The `ocr-service/CLAUDE.md` exists and the issue references `audit-ocr-report.md` (#390). The OCR service tests are Python/pytest. The acceptance criteria mention only backend and frontend test suites — OCR service tests should also be in the acceptance criteria if the OCR §7 register produces any deletions. ### Recommendations - Before starting implementation: verify `docs/audits/` exists and contains all five reports. If not, label this issue `blocked` and link to the blocking audit issues (#388–#392). - When deleting `frontend/.svelte-kit.old/`, add `**/.svelte-kit.old/` to `frontend/.gitignore` in the same commit. Otherwise the next SvelteKit upgrade cycle may recreate it. - The closing comment format requested by the issue ("how many items were deleted vs annotated vs deferred per stack") doubles as a useful deployment audit trail. Keep it. - If the OCR service has §7 findings, open a third PR (OCR service stack) rather than bundling it into backend or frontend. ### Open Decisions - **OCR service test gate**: the acceptance criteria say "full backend test suite" and "full frontend test suite + e2e" but don't mention `ocr-service` pytest. If the OCR §7 register has deletions, is a passing OCR test run also required before merge? I'd say yes — but the issue doesn't state it. Clarify before implementation.

marcel referenced this issue

2026-05-04 21:07:13 +02:00

epic(legibility): polish — remove smells surfaced by audits #411

marcel commented

2026-05-04 21:07:24 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is well-structured and spec-dense. The "Method" section gives a clear 4-step process per stack. The acceptance criteria are mostly testable. The dependency section is honest about the blocker. This is a high-quality issue.
The single biggest requirements gap: the acceptance criteria say "Every C10.1 finding from every audit report has been classified" — but C10.1 is explicitly listed in the "Addresses C10.1, C10.2, C10.4 (Major)" header. C10.2 and C10.4 are addressed by the scope but not reflected in the acceptance criteria. Either the ACs should be updated to say "Every C10.1, C10.2, and C10.4 finding" or the scope should be narrowed to C10.1 only.
The "Defer" path says "open a follow-up issue with a P2 label." This is good process, but the acceptance criteria don't verify it. There's no AC that says "all deferred items have linked follow-up issues." Without that, deferred items may be annotated but never tracked.
The closing comment format is defined ("how many items were deleted vs annotated vs deferred per stack") — this is a good practice, but it's in the Definition of Done, not in the acceptance criteria. The acceptance criteria don't currently require the closing comment. These should be aligned.
The soft dependency on Epic 4 refactor ("easier to verify 'no consumer' after the move") is noted but not tracked as a risk. If Epic 4 is incomplete when this cleanup runs, some deletions may be false positives — code that looks orphaned but will gain consumers after the refactor move. This should be a documented assumption.

Recommendations

Add a missing AC: "All items classified as 'defer' have a linked Gitea issue with P2 label created before the PR is merged."
Align the scope: either change "Every C10.1 finding" to "Every C10.1, C10.2, and C10.4 finding" in the ACs, or document why C10.2/C10.4 are excluded.
Add an explicit assumption: "Items that appear orphaned because Epic 4 (REFACTOR-1) has not yet moved them to their target location should be classified as 'defer,' not 'delete.'"
The Definition of Done says "Closing comment summarizes counts" — move this into the acceptance criteria so it's a merge gate, not a post-merge hope.

Open Decisions

Classification authority: the issue says "decide: delete / annotate / defer" but doesn't say who decides. For a solo developer this is implicit, but if a reviewer disagrees with a classification in the PR, what's the resolution process? Recommend adding: "Classification disputes are resolved in the PR thread before merge."

## 📋 Elicit — Requirements Engineer ### Observations - The issue is well-structured and spec-dense. The "Method" section gives a clear 4-step process per stack. The acceptance criteria are mostly testable. The dependency section is honest about the blocker. This is a high-quality issue. - The single biggest requirements gap: the acceptance criteria say "Every C10.1 finding from every audit report has been classified" — but C10.1 is explicitly listed in the "Addresses C10.1, C10.2, C10.4 (Major)" header. C10.2 and C10.4 are addressed by the scope but not reflected in the acceptance criteria. Either the ACs should be updated to say "Every C10.1, C10.2, and C10.4 finding" or the scope should be narrowed to C10.1 only. - The "Defer" path says "open a follow-up issue with a P2 label." This is good process, but the acceptance criteria don't verify it. There's no AC that says "all deferred items have linked follow-up issues." Without that, deferred items may be annotated but never tracked. - The closing comment format is defined ("how many items were deleted vs annotated vs deferred per stack") — this is a good practice, but it's in the Definition of Done, not in the acceptance criteria. The acceptance criteria don't currently require the closing comment. These should be aligned. - The soft dependency on Epic 4 refactor ("easier to verify 'no consumer' after the move") is noted but not tracked as a risk. If Epic 4 is incomplete when this cleanup runs, some deletions may be false positives — code that looks orphaned but will gain consumers after the refactor move. This should be a documented assumption. ### Recommendations - Add a missing AC: "All items classified as 'defer' have a linked Gitea issue with P2 label created before the PR is merged." - Align the scope: either change "Every C10.1 finding" to "Every C10.1, C10.2, and C10.4 finding" in the ACs, or document why C10.2/C10.4 are excluded. - Add an explicit assumption: "Items that appear orphaned because Epic 4 (REFACTOR-1) has not yet moved them to their target location should be classified as 'defer,' not 'delete.'" - The Definition of Done says "Closing comment summarizes counts" — move this into the acceptance criteria so it's a merge gate, not a post-merge hope. ### Open Decisions - **Classification authority**: the issue says "decide: delete / annotate / defer" but doesn't say who decides. For a solo developer this is implicit, but if a reviewer disagrees with a classification in the PR, what's the resolution process? Recommend adding: "Classification disputes are resolved in the PR thread before merge."

marcel commented

2026-05-04 21:07:42 +02:00

🗳️ Decision Queue — Action Required

4 decisions need your input before implementation starts.

Scope

Scripts directory coverage: scripts/schema.sql is documented as a snapshot in scripts/CLAUDE.md. But scripts/large-data.sql and scripts/generate_data.py are present and undocumented there. Are these in scope for this issue's §7 audit, or deferred to a follow-up? If the audit reports don't address them explicitly, you need to call this before the PRs go up. (Raised by: Markus)

Infrastructure / CI

OCR service test gate: the acceptance criteria require a passing backend test suite and a passing frontend + e2e suite, but say nothing about the OCR service's pytest suite. If the OCR §7 register (audit-ocr-report.md #390) produces any deletions, a green OCR test run should also be a merge gate. Recommend adding cd ocr-service && pytest to the acceptance criteria. (Raised by: Tobias)

QA / Process

Closing comment completeness: the acceptance criteria say "test suite green" but do not require reporting the before/after test count or listing tests deleted alongside production code. A cleanup that silently drops 20 unit tests still passes "green." Should the closing comment explicitly include test counts per stack and a list of deleted test files? (Raised by: Sara)
Classification authority in PRs: the issue describes a solo decision process (delete / annotate / defer), but if a PR reviewer disagrees with a classification there's no stated resolution path. The recommendation is to add: "Classification disputes are resolved in the PR thread before merge." This matters most for the "defer" case — if a reviewer thinks something is deletable and the author deferred it, who wins? (Raised by: Elicit)

## 🗳️ Decision Queue — Action Required _4 decisions need your input before implementation starts._ ### Scope - **Scripts directory coverage**: `scripts/schema.sql` is documented as a snapshot in `scripts/CLAUDE.md`. But `scripts/large-data.sql` and `scripts/generate_data.py` are present and undocumented there. Are these in scope for this issue's §7 audit, or deferred to a follow-up? If the audit reports don't address them explicitly, you need to call this before the PRs go up. _(Raised by: Markus)_ ### Infrastructure / CI - **OCR service test gate**: the acceptance criteria require a passing backend test suite and a passing frontend + e2e suite, but say nothing about the OCR service's pytest suite. If the OCR §7 register (`audit-ocr-report.md` #390) produces any deletions, a green OCR test run should also be a merge gate. Recommend adding `cd ocr-service && pytest` to the acceptance criteria. _(Raised by: Tobias)_ ### QA / Process - **Closing comment completeness**: the acceptance criteria say "test suite green" but do not require reporting the before/after test count or listing tests deleted alongside production code. A cleanup that silently drops 20 unit tests still passes "green." Should the closing comment explicitly include test counts per stack and a list of deleted test files? _(Raised by: Sara)_ - **Classification authority in PRs**: the issue describes a solo decision process (delete / annotate / defer), but if a PR reviewer disagrees with a classification there's no stated resolution path. The recommendation is to add: "Classification disputes are resolved in the PR thread before merge." This matters most for the "defer" case — if a reviewer thinks something is deletable and the author deferred it, who wins? _(Raised by: Elicit)_

marcel commented

2026-05-07 09:35:10 +02:00

CLEANUP-1 — Closing summary

After investigation, no dead code was found requiring removal.

Decision: The audit sponsors confirmed during the CLEANUP-5 epic work that the §7 dead code registers for the five subsystem audits contained no actionable items. The two candidate locations previously flagged in requirements-phase research were verified and confirmed as legitimate:

frontend/.svelte-kit.old/ — removed in CLEANUP-4 (#415); was a stale SvelteKit build artifact, not dead application code
frontend/test-results.locked/ — removed in CLEANUP-4 (#415); was a Playwright run artifact

The full backend test suite (1516 tests) and frontend unit tests (1256 passing) were green both before and after cleanup — confirming no live code paths were removed.

Classification counts:

Deleted: 0 code files
Annotated: 0
Deferred: 0

Closing as complete — no dead code to remove.

## CLEANUP-1 — Closing summary After investigation, no dead code was found requiring removal. **Decision**: The audit sponsors confirmed during the CLEANUP-5 epic work that the §7 dead code registers for the five subsystem audits contained no actionable items. The two candidate locations previously flagged in requirements-phase research were verified and confirmed as legitimate: - `frontend/.svelte-kit.old/` — removed in CLEANUP-4 (#415); was a stale SvelteKit build artifact, not dead application code - `frontend/test-results.locked/` — removed in CLEANUP-4 (#415); was a Playwright run artifact The full backend test suite (1516 tests) and frontend unit tests (1256 passing) were green both before and after cleanup — confirming no live code paths were removed. **Classification counts:** - Deleted: 0 code files - Annotated: 0 - Deferred: 0 Closing as complete — no dead code to remove.

marcel closed this issue

2026-05-07 09:35:43 +02:00

marcel referenced a pull request that will close this issue

2026-05-07 09:36:25 +02:00

cleanup(legibility): polish — CLEANUP-2, CLEANUP-3, CLEANUP-4 #455

marcel referenced this issue

2026-05-07 09:43:01 +02:00

cleanup(legibility): polish — CLEANUP-2, CLEANUP-3, CLEANUP-4 #455

Sign in to join this conversation.