epic(legibility): polish — remove smells surfaced by audits #411
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Epic context
This is Epic 5 of 5 for the Codebase Legibility Refactor (see #387).
After Epic 4 has restructured the code, Epic 5 cleans up everything the audits found that wasn't structural: dead code (C10.1), vague or obsolete comments (C7.3), naming violations (C4.3), and repo-root hygiene (C1.5). Then CLEANUP-5 re-runs the readiness scorecard to confirm the codebase is ready for evaluation.
Scope
Acceptance criteria
docs/LEGIBILITY-READINESS.md(refreshed by CLEANUP-5) gives 🟢 verdicts on §5 (refactor readiness) AND §6 (evaluation readiness)Definition of Done
CLEANUP-5 closes with a 🟢🟢 verdict. The codebase is ready to send to Anja and Tobias.
Dependency
👨💻 Felix Brandt — Senior Fullstack Developer
Observations
CLEANUP-2 (vague comments) — The two confirmed TODOs I found in the current codebase are:
frontend/src/routes/admin/+layout.server.ts:29— a TODO to replace a fat user-load with a dedicated stats endpoint. This is a real action item, not just a comment smell; it needs a linked issue before it's removed.frontend/src/lib/components/chronik/ChronikRow.svelte:163— a TODO noting the backend doesn't yet expose a comment body preview. This is a cross-domain note about a missing API feature; it needs a linked issue.CLEANUP-3 (naming violations) — Only one confirmed violator in main source:
SecurityUtils.java. It's afinalclass with a private constructor and one static method. This is the classic "I didn't know where else to put this" pattern. The right fix is to dissolve it: moverequireUserId()logic into a@ComponentlikeAuthenticatedUserResolveror integrate it into thePermissionAspect. A static utils class breaks testability and signals a missing abstraction.CLEANUP-1 (dead code) — Both files flagged in my search (
DocumentController.java,ThumbnailService.java) turn out to contain legitimate comments explaining design decisions, not actual dead code. The audit reports (not yet written) will surface the real list. CLEANUP-1 should not start until AUDIT-1 and AUDIT-2 produce that list.Ordering risk — The epic notes CLEANUP-5 is blocked by Epic 1 (audits) and Epic 4 (refactor). But CLEANUP-1 through CLEANUP-4 are listed here as if they can start now. They shouldn't — CLEANUP-1 specifically requires the audit findings as input. Child issues #412–#415 should carry explicit "Blocked by #388/#389" dependencies.
Recommendations
depends_on: [#388, #389](audit-frontend, audit-backend) to child issues #412 and #413 before any implementation starts. Running cleanup blind risks removing code the audit would have explained.SecurityUtils.java: rename toAuthenticationHelper— wait, that's still a violation. The right move isCurrentUserResolveras a Spring@Component, injected where needed. This turns a static untestable util into a mockable dependency, which is a testability improvement, not just a rename.docs/LEGIBILITY-READINESS.mdrefresh) depends on all prior cleanup being done. Verify that #416 (scorecard re-run) lists all four cleanup issues as blockers, not just the refactor epic.🏛️ Markus Keller — Application Architect
Observations
CLEANUP-3 is the structurally most interesting task.
SecurityUtils.javais a static utility class with one method that resolves an authenticated user from Spring Security'sAuthentication. Static helpers that bridge framework and domain are a coupling antipattern: they cannot be mocked, they mix framework concerns into domain logic, and they suppress the real architectural question — who owns "resolve current user"? The answer isSecurityConfigandPermissionAspectown that concern. A@Component CurrentUserResolverwithrequireUserId(Authentication)as an instance method would be injectable, mockable, and correctly scoped.CLEANUP-4 repo hygiene has a scoping ambiguity. The
.gitignorealready contains.worktrees/but the git status shows.claude/worktrees/(with the.claude/prefix) as untracked — and.agent/is also untracked at repo root. The issue title says "ignore .agent/.worktrees" but the current.gitignoreonly covers.worktrees/(top-level), not.agent/or.claude/worktrees/. The fix requires two lines:.agent/and.claude/worktrees/(or a glob). Check whether.claude/settings.jsonand.claude/personas/should also be ignored or committed — that's a deliberate choice, but it needs to be stated explicitly.proofshot-artifacts/ retention policy is undefined. Currently 44 files in 5 directories, 7.6MB. Without a policy, this grows unboundedly and clutters the repo root for any external developer. The clean approach: either commit a
.gitignoreentry forproofshot-artifacts/entirely (they are dev-time verification artifacts, not source), or define a rolling retention (keep last N runs, delete older ones via a script). The first option is simpler and matches how.vitest-attachmentsis handled.CLEANUP-5's Definition of Done references
docs/LEGIBILITY-READINESS.md— this file doesn't exist yet; it's created by AUDIT-6 (#393). The scorecard re-run in CLEANUP-5 (#416) refreshes it after cleanup. This dependency chain is correct and should be documented as#416 blocked by #393 AND #412–#415.The epic has no milestone link visible in the issue body. It is in the "Codebase Legibility" milestone (confirmed via issue list), but the acceptance criteria reference CLEANUP-5 closing with a "🟢🟢 verdict" — this verdict needs to be defined in #416's body, not just in this epic. If the scorecard criteria drift, the epic's DoD becomes unverifiable.
Recommendations
.agent/vs.claude/worktrees/ambiguity before CLEANUP-4 starts. Add both to.gitignore:.agent/and.claude/worktrees/. Decide and document whether.claude/personas/,.claude/settings.json, and.claude/skills/are project configuration (commit) or developer-local (ignore).proofshot-artifacts/: add to.gitignore. These are transient verification screenshots, not project history. Ascripts/clean-proofshots.shscript that purges everything older than 30 days is a reasonable complement.SecurityUtils.javadissolution belongs in CLEANUP-3, not CLEANUP-1. It's not dead code — it's a naming and structural violation. Make sure #414 (naming cleanup) covers it explicitly.#412 AND #413 AND #414 AND #415 AND #393as prerequisites. Otherwise the scorecard re-run might start before cleanup is complete.🔒 Nora "NullX" Steiner — Application Security Engineer
Observations
SecurityUtils.javais a security-adjacent class with a naming smell and a design smell. The staticrequireUserId()method does two things: it validates authentication state and it resolves anAppUserfrom the database. This dual responsibility means the authentication guard and the user lookup failure mode are tested together or not at all. If this class is missed during cleanup, it remains an untestable static dependency — which means security fixes to it cannot be covered by failing unit tests first.The "remove 'ask Marcel' markers" task (CLEANUP-2) should include a sweep for any TODOs that defer security decisions. I found no
// ask Marcelmarkers in the current source, but the two live TODOs are worth scrutinizing:admin/+layout.server.ts:29— the TODO defers creation of a stats endpoint that returns "counts only." The current code apparently loads full user/group data for a stats display. If it's loading more data than needed and exposing it to the admin template, that's a data-minimization finding worth noting in the audit before cleanup.ChronikRow.svelte:163— notes missing comment body preview. Low security relevance.CLEANUP-5's scorecard should explicitly include a security readiness dimension. The current acceptance criteria check §5 (refactor readiness) and §6 (evaluation readiness) of the scorecard. If the rubric (C1–C10) includes security-related categories (C7/C10 look like candidates based on the issue text), those categories need 🟢 verdicts too before the codebase goes to Anja and Tobias. The DoD says "ready to send to Anja and Tobias" — make sure the scorecard has a security section, not just structural checks.
.agent/directory at repo root — containscurrent-plan.md,PLAN.md, and multipleagent-*subdirectories. These directories may contain implementation plans that reference internal architecture decisions, credentials placeholders, or PII from the family archive use case. Before adding.agent/to.gitignore, verify that none of the existing files contain information that shouldn't be in git history (they're currently untracked, so git history is clean). Adding.agent/**to.gitignoreis the right call.Recommendations
admin/+layout.server.ts:29's load function to confirm it isn't over-fetching sensitive user data for a stats panel. If it is, the TODO cleanup should include a note that the over-fetch is a data minimization finding — not just a code style issue..agent/to.gitignorenow (before CLEANUP-4 runs), and do a quick content scan of.agent/*.mdfor any credentials or sensitive paths before the directory grows further. This is a low-effort, low-risk win.SecurityUtils.javadissolution in CLEANUP-3 must include a test:currentUserResolver_throws_unauthorized_when_authentication_is_null(). The test goes red, the rename/refactor goes green. No security class change without a test.🧪 Sara Holt — QA Engineer & Test Strategist
Observations
CLEANUP-1 (dead code removal) has no verification mechanism defined. Removing code without a test to confirm the deleted path was never reached is a silent regression risk. The standard approach: run mutation testing (PIT) before deletion to confirm the code is truly unreachable, then run the full test suite after deletion. If the suite still passes, the deletion was safe. The epic should specify this verification step.
CLEANUP-5 (scorecard re-run) is the closest thing to a quality gate in this epic, but the acceptance criteria are written at the epic level rather than the child issue level.
#416needs explicit, testable criteria: which sections of the scorecard must show 🟢, and what would trigger a 🟡 that still closes the epic vs. a 🔴 that blocks closure. Without these thresholds, "ratify ready for evaluation" is a judgment call rather than a measurable gate.No regression test coverage is specified for any of the five CLEANUP tasks. This is expected for documentation and gitignore tasks, but CLEANUP-1 (delete code) and CLEANUP-3 (rename class) are structural changes that can break compilation and behavior. CLEANUP-3 in particular will touch import statements across multiple files when
SecurityUtilsis dissolved. The test suite run after CLEANUP-3 needs to explicitly verify thatSecurityUtilsTest.java(which exists per my codebase search) was updated or migrated, not just deleted.The two live TODOs I found in the frontend are edge cases for CLEANUP-2's scope. They're well-formed TODOs with context, not "ask Marcel" markers. If CLEANUP-2 deletes them without creating linked issues, those backend features become invisible debt. The child issue #413 should specify: replace actionable TODOs with linked Gitea issues before deleting the comment.
CLEANUP-4 gitignore changes should be followed by
git statusverification as part of the DoD for #415. After adding.agent/and.claude/worktrees/to.gitignore, the untracked files count should drop to zero for those directories. This is a two-line verify step but it's easy to miss if the gitignore glob is wrong (e.g.,.worktrees/covers top-level but not.claude/worktrees/).Recommendations
./mvnw testbefore and after each deletion. Confirm no test failures. For critical service methods, run PIT mutation test first to verify they are genuinely unreachable."// See #NNNreference before committing the cleanup."SecurityUtils.java, migrateSecurityUtilsTest.javato cover the newCurrentUserResolvercomponent. Do not delete the test without replacement."git statusafter gitignore changes and confirm.agent/,.claude/worktrees/, andproofshot-artifacts/no longer appear as untracked."🚀 Tobias Wendt — DevOps & Platform Engineer
Observations
CLEANUP-4 gitignore is more nuanced than the title suggests. The current
.gitignorehas.worktrees/but the untracked directories include.agent/(repo root) and.claude/worktrees/(under.claude/, not root). These are different paths..worktrees/catches a top-level.worktrees/directory — it does NOT catch.claude/worktrees/. This is why.claude/worktrees/still shows as untracked. The fix needs two explicit entries:Additionally,
.claude/scheduled_tasks.lockis untracked — that's a lock file that should never be committed. Add*.lockentries or specifically.claude/*.lock.proofshot-artifacts/at repo root is 7.6MB across 44 files and growing. This is a developer tooling artifact directory with no defined retention policy. For a solo developer who will send this codebase to external reviewers (Anja and Tobias), having unexplained screenshot directories at the repo root is confusing. My recommendation: addproofshot-artifacts/to.gitignorenow, and optionally add ascripts/clean-e2e-data.sh-style companion script (scripts/clean-proofshots.sh) that deletes all but the last run. This is a one-liner:find proofshot-artifacts/ -mindepth 1 -maxdepth 1 -not -newer proofshot-artifacts/$(ls -t proofshot-artifacts/ | head -1) -exec rm -rf {} +..claude/settings.jsonis untracked. This may be intentional (personal Claude Code settings) or accidental. If it contains project-level tool permissions (like thefewer-permission-promptsskill would generate), it's project configuration and should be committed. If it contains local paths or personal preferences, it should be in.gitignore. This needs an explicit decision — currently it's in neither place, which is a hygiene gap.node_modules/at repo root is untracked. Check: is there apackage.jsonat repo root? If so, this is an accidental npm init or a workspace setup. The git status also showspackage.jsonandpackage-lock.jsonat repo root. If these are intentional (e.g., a monorepo workspace), they need documentation. If they're accidental, they need deletion plus a.gitignoreentry.infra/directory is untracked at repo root. Given the stack has adocker-compose.ymlat root, I'd expect infrastructure config to live there, not in a separateinfra/directory. This either belongs committed (if it contains useful deployment configs) or gitignored (if it's a generated/local artifact).Recommendations
.agent/,.claude/worktrees/,.claude/scheduled_tasks.lock,proofshot-artifacts/, and the orphanednode_modules/+package.jsonat root. The current issue title only mentions.agent/.worktrees— this is narrower than what the git status shows..claude/settings.json,.claude/personas/,.claude/skills/? These are project-level AI configuration — a case can be made for committingpersonas/andskills/(they define the team's workflow) while ignoringsettings.json(personal tool config). Document this decision in CLEANUP-4's description.infra/and the rootpackage.json/package-lock.jsonbefore they accumulate further. If they're intentional, commit with a comment indocker-compose.ymlorREADME.mdexplaining them. If accidental, delete and gitignore before the external review.📋 Elicit — Requirements Engineer
Observations
The five child issues (#412–#416) are well-structured relative to the milestone's overall shape, but they have an ordering problem embedded in the epic body that the issue descriptions don't surface: CLEANUP-1 through CLEANUP-4 need the audit outputs (Epic 1) as their input, yet they carry no explicit blocked-by links to #388–#392. A reader picking up #412 in isolation cannot tell it must wait for the audit reports.
CLEANUP-5 acceptance criteria mix two different concerns. "Ratify 'ready for evaluation'" is a conclusion, not a criterion. The measurable criteria should be:
docs/LEGIBILITY-READINESS.mdexists and was last updated after CLEANUP-4 closed.Without these specifics, "🟢🟢 verdict" is a subjective assessment rather than a binary gate.
The Definition of Done says "The codebase is ready to send to Anja and Tobias." This is the real exit criterion for the entire Codebase Legibility milestone, but it's stated only in this epic's DoD, not in any child issue. If Epic 5 closes without that final check being done, the milestone could close prematurely. Consider making this the milestone-level acceptance criterion, not just the epic's DoD.
CLEANUP-2's scope is ambiguous. "Rewrite vague comments and remove 'ask Marcel' / 'Claude generated' markers" covers two different activities: (a) improve comment quality and (b) remove AI attribution markers. These have different verification methods: (a) requires human judgment, (b) can be verified with a
grep. The child issue #413 should split these into separate checklists so each can be mechanically verified.CLEANUP-4 scope is too narrow given what git status shows. The issue covers "proofshot-artifacts retention, ignore .agent/.worktrees" but the actual hygiene surface is larger (see Tobias's comment). The issue title should be updated to reflect the full scope, or sub-tasks should be listed explicitly.
Recommendations
.agent/,.claude/worktrees/,.claude/scheduled_tasks.lock,proofshot-artifacts/, and the decision on rootpackage.json/infra/.🎨 Leonie Voss — UX Designer & Accessibility Strategist
No direct UI/UX concerns for this epic — CLEANUP-1 through CLEANUP-4 are all backend/infra/tooling hygiene tasks with no frontend component changes.
What I checked: none of the named cleanup tasks touch Svelte components, CSS, or Tailwind tokens.
SecurityUtils.javadissolution (CLEANUP-3) has no frontend surface. The gitignore changes (CLEANUP-4) and dead code removal (CLEANUP-1) don't affect the rendered UI.One observation that's adjacent to UX: the
ChronikRow.svelteTODO (flagged by Felix) notes the backend doesn't expose a comment body preview. When that TODO is cleaned up in CLEANUP-2, whoever resolves it should ensure the linked Gitea issue specifies the display requirement, not just the API requirement: a comment preview in the Chronik row needs to handle truncation gracefully (line clamp, aria-label with full text), especially for the 60+ audience reading on small screens. That's a Medium-priority UX note to carry into the future feature issue — not a blocker for CLEANUP-2.🗳️ Decision Queue — Action Required
3 decisions need your input before CLEANUP-4 (#415) starts.
Repo Hygiene
[
.claude/directory strategy] —.claude/personas/,.claude/skills/, and.claude/settings.jsonare all untracked. Two options: (A) commitpersonas/andskills/as project configuration (they define the team's AI workflow) while addingsettings.jsonandworktrees/to.gitignore; (B) ignore the entire.claude/directory as developer-local tooling. Option A is recommended — the personas and skills are meaningful project artifacts that external reviewers Anja and Tobias could read to understand how the team works. Option B treats all Claude tooling as ephemeral. (Raised by: Markus, Tobias)[
infra/directory at repo root] — Aninfra/directory is untracked at the repo root. Is this intentional infrastructure configuration that should be committed alongsidedocker-compose.yml, or was it accidentally created and should be deleted and gitignored? There's no record in CLAUDE.md or any existing docs of whatinfra/contains or why it's separate from the existingdocker-compose.yml. (Raised by: Tobias)Architecture
package.json/package-lock.json] — There is apackage.jsonandpackage-lock.jsonat repo root (separate fromfrontend/package.json). This suggests either an accidentalnpm initor an intentional npm workspaces setup. If accidental: delete and gitignorenode_modules/. If intentional: document the workspace structure in CLAUDE.md and explain the relationship tofrontend/package.json. The current state — untracked, undocumented — is a hygiene gap that will confuse external reviewers. (Raised by: Tobias)