From d83d88d7c8b0443dfa68c514a2180d24dc386454 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 4 Sep 2026 22:16:29 +0200 Subject: [PATCH 1/9] Development: Add agent skills and a Work with AI documentation section CLAUDE.md is loaded on every request, so it has to stay short. That makes it a good place for facts and a bad place for procedures. Seven agent skills now carry the procedures, loading only when used: e2e-pr-check run only the specs a change affects ci-triage classify a red build before changing code server-arch-gates the rules a server change must satisfy liquibase-migration changelogs that survive a rolling deploy client-conventions signal APIs, cloning, TUM UI styling write-tests base classes and the test commands that mislead local-setup fresh clone to a running server and client They live at skills/ so `npx skills add ls1intum/Artemis` reaches any agent, and .claude-plugin/ presents the repository as one plugin so Claude Code can install namespaced, versioned skills. Supporting changes: - run-e2e-tests-local-fast.sh and the multinode-fast variant take --specs, which replaces the hardcoded e2e positional argument. Until now --filter mapped to Playwright --grep, which matches test titles, so there was no way to ask either runner for a set of spec files. - determine-relevant-tests.sh re-execs under bash 4+ when it finds itself on macOS's bash 3.2, where it died on declare -A. No change on CI, which already runs bash 5. - check_skill_references.py fails the Quality workflow when a skill cites a repository path that no longer exists. A skill naming a moved file is worse than no skill: an agent acts on it without checking. --- .ci/E2E-tests/determine-relevant-tests.sh | 16 ++ .claude-plugin/marketplace.json | 19 ++ .claude-plugin/plugin.json | 13 ++ .github/workflows/ci-quality.yml | 20 ++ AGENTS.md | 2 + CLAUDE.md | 14 ++ documentation/docs/developer/work-with-ai.mdx | 178 ++++++++++++++++++ run-e2e-tests-local-fast.sh | 31 ++- run-e2e-tests-local-multinode-fast.sh | 29 ++- skills/README.md | 70 +++++++ skills/ci-triage/SKILL.md | 70 +++++++ .../reference/known-failure-patterns.md | 116 ++++++++++++ skills/client-conventions/SKILL.md | 103 ++++++++++ .../reference/migration-recipes.md | 153 +++++++++++++++ skills/e2e-pr-check/SKILL.md | 115 +++++++++++ skills/liquibase-migration/SKILL.md | 73 +++++++ .../reference/migration-patterns.md | 110 +++++++++++ skills/local-setup/SKILL.md | 101 ++++++++++ skills/server-arch-gates/SKILL.md | 85 +++++++++ skills/server-arch-gates/reference/gates.md | 125 ++++++++++++ skills/write-tests/SKILL.md | 68 +++++++ skills/write-tests/reference/client.md | 75 ++++++++ skills/write-tests/reference/server.md | 68 +++++++ supporting_scripts/check_skill_references.py | 113 +++++++++++ 24 files changed, 1762 insertions(+), 5 deletions(-) create mode 100644 .claude-plugin/marketplace.json create mode 100644 .claude-plugin/plugin.json create mode 100644 documentation/docs/developer/work-with-ai.mdx create mode 100644 skills/README.md create mode 100644 skills/ci-triage/SKILL.md create mode 100644 skills/ci-triage/reference/known-failure-patterns.md create mode 100644 skills/client-conventions/SKILL.md create mode 100644 skills/client-conventions/reference/migration-recipes.md create mode 100644 skills/e2e-pr-check/SKILL.md create mode 100644 skills/liquibase-migration/SKILL.md create mode 100644 skills/liquibase-migration/reference/migration-patterns.md create mode 100644 skills/local-setup/SKILL.md create mode 100644 skills/server-arch-gates/SKILL.md create mode 100644 skills/server-arch-gates/reference/gates.md create mode 100644 skills/write-tests/SKILL.md create mode 100644 skills/write-tests/reference/client.md create mode 100644 skills/write-tests/reference/server.md create mode 100644 supporting_scripts/check_skill_references.py diff --git a/.ci/E2E-tests/determine-relevant-tests.sh b/.ci/E2E-tests/determine-relevant-tests.sh index b4494e38ff70..4edc8cb134b0 100755 --- a/.ci/E2E-tests/determine-relevant-tests.sh +++ b/.ci/E2E-tests/determine-relevant-tests.sh @@ -8,6 +8,22 @@ set -e +# This script uses associative arrays and `mapfile`, both of which need bash 4+. CI runners ship +# bash 5, but macOS still ships bash 3.2 as /bin/bash, where the script dies on `declare -A` with a +# misleading "invalid option" error. Re-exec under a newer bash when one is on PATH (Homebrew +# installs it as /opt/homebrew/bin/bash) so the script is usable locally, which is what the +# e2e-pr-check agent skill and anyone debugging test selection needs. +if [ -z "${DETERMINE_RELEVANT_TESTS_REEXEC:-}" ] && [ "${BASH_VERSINFO[0]}" -lt 4 ]; then + for candidate in "$(command -v bash || true)" /opt/homebrew/bin/bash /usr/local/bin/bash; do + if [ -x "$candidate" ] && [ "$("$candidate" -c 'echo ${BASH_VERSINFO[0]}')" -ge 4 ]; then + DETERMINE_RELEVANT_TESTS_REEXEC=1 exec "$candidate" "${BASH_SOURCE[0]}" "$@" + fi + done + echo "ERROR: this script needs bash 4 or newer (found $BASH_VERSION)." >&2 + echo "On macOS: brew install bash" >&2 + exit 1 +fi + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" MAPPING_FILE="$SCRIPT_DIR/e2e-test-mapping.json" REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json new file mode 100644 index 000000000000..7ce0456d8ab6 --- /dev/null +++ b/.claude-plugin/marketplace.json @@ -0,0 +1,19 @@ +{ + "name": "artemis", + "description": "Agent skills for developing Artemis, the interactive learning platform.", + "owner": { + "name": "Artemis Team", + "url": "https://github.com/ls1intum/Artemis" + }, + "plugins": [ + { + "name": "artemis", + "source": "./", + "description": "Agent skills for developing Artemis: run the E2E tests a change affects, triage red CI, write migrations and tests that pass first time, and follow the server and client conventions the build enforces.", + "category": "development", + "homepage": "https://docs.artemis.tum.de/developer/work-with-ai", + "repository": "https://github.com/ls1intum/Artemis", + "license": "MIT" + } + ] +} diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json new file mode 100644 index 000000000000..44af8cb14a5b --- /dev/null +++ b/.claude-plugin/plugin.json @@ -0,0 +1,13 @@ +{ + "name": "artemis", + "description": "Agent skills for developing Artemis: run the E2E tests a change affects, triage red CI, write migrations and tests that pass first time, and follow the server and client conventions the build enforces.", + "version": "1.0.0", + "author": { + "name": "Artemis Team", + "url": "https://github.com/ls1intum/Artemis" + }, + "homepage": "https://docs.artemis.tum.de/developer/work-with-ai", + "repository": "https://github.com/ls1intum/Artemis", + "license": "MIT", + "keywords": ["artemis", "spring-boot", "angular", "playwright", "liquibase", "archunit"] +} diff --git a/.github/workflows/ci-quality.yml b/.github/workflows/ci-quality.yml index dd4763b356d2..bfa8ace93bb0 100644 --- a/.github/workflows/ci-quality.yml +++ b/.github/workflows/ci-quality.yml @@ -267,3 +267,23 @@ jobs: echo "::error::Query Quality failed. See https://docs.artemis.tum.de/developer/guidelines/performance and https://docs.artemis.tum.de/developer/guidelines/database." exit 1 fi + + agent-skills: + name: Agent Skills + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + - name: Checkout code + uses: actions/checkout@v6 + with: + ref: ${{ inputs.commit_sha }} + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.14' + # A skill that cites a file which has since moved is worse than no skill: an agent acts on it + # without checking. This is a pure source scan, so it is cheap and always runs. + - name: Skill path references + run: python supporting_scripts/check_skill_references.py diff --git a/AGENTS.md b/AGENTS.md index 73662c2b6e98..ea675074313c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,3 +1,5 @@ # Repository Guidelines Before making changes, read and follow **[CLAUDE.md](./CLAUDE.md)** in full; it is the single source of truth for AI coding assistants working in this repository. + +For task-specific procedures, install the repository's agent skills with `npx skills add ls1intum/Artemis`. They cover running the E2E tests a change affects, triaging a red build, database migrations, the server architecture gates, the client conventions, writing tests, and local setup. See [`skills/`](./skills/) and [`documentation/docs/developer/work-with-ai.mdx`](./documentation/docs/developer/work-with-ai.mdx). diff --git a/CLAUDE.md b/CLAUDE.md index 3a238a8f1397..633b781440e9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,6 +2,20 @@ This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +## Agent skills + +This file holds **facts** about the repository. **Procedures** live in [`skills/`](./skills/) as agent skills, which load only when used and can therefore go into far more depth than this file can afford. Install them with `npx skills add ls1intum/Artemis`, or in Claude Code with `/plugin marketplace add ls1intum/Artemis` followed by `/plugin install artemis@artemis`. + +- `e2e-pr-check` — run only the Playwright specs a change affects, and read the result correctly +- `ci-triage` — classify a red build before changing any code +- `server-arch-gates` — the architectural rules a server change must satisfy, and how to check each locally +- `liquibase-migration` — write a changelog that survives a rolling deploy on both databases +- `client-conventions` — Angular signal APIs, cloning, template control flow, TUM UI styling +- `write-tests` — base class selection and the test commands that silently do the wrong thing +- `local-setup` — fresh clone to a running server and client + +When a convention below changes, update the corresponding skill in the same pull request. See [`documentation/docs/developer/work-with-ai.mdx`](./documentation/docs/developer/work-with-ai.mdx). + ## Project Overview Artemis is an interactive learning platform for programming exercises, quizzes, modeling tasks, and exams with automatic and manual assessment. It integrates with AI services (Iris for virtual tutoring, Athena for automated assessment, Hyperion for exercise creation). diff --git a/documentation/docs/developer/work-with-ai.mdx b/documentation/docs/developer/work-with-ai.mdx new file mode 100644 index 000000000000..94a457d160f1 --- /dev/null +++ b/documentation/docs/developer/work-with-ai.mdx @@ -0,0 +1,178 @@ +--- +id: work-with-ai +title: Work with AI +description: Install and use the Artemis agent skills, which teach an AI coding agent how this repository actually works. +--- + +import Callout from '../../src/components/Callout/Callout'; +import { CalloutVariant } from '../../src/components/Callout/Callout.types'; + +# Work with AI + +Artemis ships a set of **agent skills**: packaged procedures that teach an AI coding agent how this +repository actually works. Which Playwright specs a change needs. Why a build went red. Which +architectural rules a new service is subject to, and the command that proves each one. + +They live in [`skills/`](https://github.com/ls1intum/Artemis/tree/develop/skills) and work with +Claude Code, Cursor, Codex, GitHub Copilot, opencode, Zed, and around seventy other agents. + +## Why skills and not just CLAUDE.md + +`CLAUDE.md` is loaded into the agent's context on every request, so it has to stay short. That makes +it a good place for facts and rules and a bad place for procedures. + +A skill is the opposite. Only its one-line description stays resident; the body loads when the agent +decides the skill is relevant. So a skill can afford to be long, and can carry the things that never +fit into `CLAUDE.md`: the steps, the exact commands, the reason behind a rule, and the failure modes +that look like something else. + +The division is: + +- **`CLAUDE.md`**: what is true about this repository. +- **`skills/`**: how to do a particular job in it. + +## Installing + +### Any agent + +```bash +npx skills add ls1intum/Artemis +``` + +This installs the skills into the agent's own skills directory for this project. It supports Claude +Code, Cursor, Codex, Copilot, Cline, opencode, Zed, Windsurf, Gemini CLI, and many more. + +### Claude Code + +Claude Code can install them as a versioned plugin, which namespaces the skills and lets you update +them with a single command: + +``` +/plugin marketplace add ls1intum/Artemis +/plugin install artemis@artemis +``` + +The skills then appear as `/artemis:e2e-pr-check`, `/artemis:ci-triage`, and so on. Claude also +invokes them on its own when a task matches a skill's description; you do not have to name them. + + + + Prefer the plugin form over `npx` if you use Claude Code. A personal skill in `~/.claude/skills/` + with the same name as a project skill takes precedence over it, and the namespaced plugin form + cannot be shadowed that way. + + + +### Working on the skills themselves + +```bash +claude --plugin-dir . +``` + +This loads your working copy directly, with no install step, so you can edit a `SKILL.md` and try it +immediately. + +## The skills + +| Skill | Use it when | +| --------------------- | ---------------------------------------------------------------------------- | +| `e2e-pr-check` | You want the E2E tests a change affects, not all 316 of them | +| `ci-triage` | A pull request is red, or a check never appeared | +| `server-arch-gates` | You changed Java under `src/main/java` and want the gates to pass first time | +| `liquibase-migration` | You are adding, changing, or dropping anything in the database schema | +| `client-conventions` | You are writing or migrating Angular code | +| `write-tests` | You are adding a JUnit test or a Vitest spec | +| `local-setup` | You are setting up from a fresh clone, or something will not start | + +### Example prompts + +``` +Run the E2E tests my branch affects +Why is PR 13652 red? +Add a NOT NULL constraint on complaint.result_id +Migrate this component to signal inputs +Which architecture tests does this new service need to pass? +``` + +You do not need to name a skill. The agent selects one from its description, the same way it picks +any other tool. Naming it explicitly (`/artemis:ci-triage`) forces the choice when you want to. + +## What each skill covers + +**`e2e-pr-check`** resolves the affected specs with +[`.ci/E2E-tests/determine-relevant-tests.sh`](https://github.com/ls1intum/Artemis/blob/develop/.ci/E2E-tests/determine-relevant-tests.sh), +the same resolver CI uses, so local and CI selection cannot disagree. It then picks between the +single-node and multi-node runner, runs with `--specs`, and interprets the outcome: whether the +failure already exists on develop, whether the client under test is really your branch, and why an +assertion on a shared counter must be a lower bound. + +**`ci-triage`** classifies a failure before anyone edits code. Server Tests reporting no failures +next to a timeout is a healthy run killed by the backstop. One ArchUnit violation reds two jobs. A +conflicting pull request starts no CI at all, which looks exactly like a dropped event. + +**`server-arch-gates`** maps a change to the rules it must satisfy: no transaction boundaries in +services, no direct `EntityManager` or JDBC, all cross-node state through `DistributedDataProvider`, +no Hibernate second-level cache, and the counted gates that sit at their limit. + +**`liquibase-migration`** covers the guarded pattern for adding a NOT NULL constraint, expand and +contract for rolling deployments, and why triggers are not an option here. + +**`client-conventions`** covers signal APIs, the `ngOnChanges` ban, `@if` and `@for`, `deepClone` +rather than spread or `structuredClone`, and TUM UI with semantic colour tokens. + +**`write-tests`** covers base class selection, the admin naming rule that forces a shared +`@ResourceLock`, and the Vitest invocations that quietly do the wrong thing. + +**`local-setup`** takes a fresh clone to a running server and client. + +## Contributing a skill + +A skill is a directory under `skills/` with a `SKILL.md` and optional `reference/` files: + +``` +skills/ + my-skill/ + SKILL.md + reference/background.md +``` + +```markdown +--- +name: my-skill +description: What it does and when to use it, key use case first. +--- + +# Title + +The procedure. +``` + +The `description` is the only part always in context, and it is what the agent selects on. Write it +as "what it does and when to use it", not as a title. + +Four rules for this repository: + +1. **Every factual claim cites a repository path.** + [`supporting_scripts/check_skill_references.py`](https://github.com/ls1intum/Artemis/blob/develop/supporting_scripts/check_skill_references.py) + runs in the Quality workflow and fails if a cited path no longer exists. +2. **Keep the body a procedure.** Background goes in a `reference/` file, which costs nothing until + the agent reads it. +3. **Say why, not just what.** A rule without its reason gets worked around instead of followed. +4. **Do not restate `CLAUDE.md`.** Add the part that does not fit there. + +Before opening a pull request: + +```bash +claude --plugin-dir . +python3 supporting_scripts/check_skill_references.py +claude plugin validate . +``` + + + + A skill that names a command or a file that has moved is worse than no skill: it is confidently + wrong, and an agent will act on it without checking. When you change a convention, update the + skill in the same pull request. That is the whole reason these live in this repository rather + than in one of their own. + + diff --git a/run-e2e-tests-local-fast.sh b/run-e2e-tests-local-fast.sh index fdf374550a05..9db52645b05d 100755 --- a/run-e2e-tests-local-fast.sh +++ b/run-e2e-tests-local-fast.sh @@ -13,6 +13,11 @@ set -e # Options: # --stop Kill server, client, and database; exit # --filter Run only tests matching the pattern (e.g., "Quiz") +# --specs "" Run only these spec paths, relative to src/test/playwright +# (e.g., "e2e/exam/ExamResults.spec.ts e2e/lecture/"). +# Replaces the default "run everything under e2e/". +# Combines with --filter. Get the paths for a branch with +# .ci/E2E-tests/determine-relevant-tests.sh # --skip-server Reuse already-running server # --skip-client Reuse already-running client # --skip-db Reuse already-running Postgres @@ -38,6 +43,7 @@ SKIP_CLIENT=false SKIP_DB=false DEBUG=false TEST_FILTER="" +TEST_SPECS="" PLAYWRIGHT_EXTRA_ARGS=() export PLAYWRIGHT_VIDEO_MODE="${PLAYWRIGHT_VIDEO_MODE:-off}" export PLAYWRIGHT_COVERAGE="${PLAYWRIGHT_COVERAGE:-off}" @@ -79,7 +85,17 @@ while [[ $# -gt 0 ]]; do TEST_FILTER="$2" shift 2 ;; - --help) head -25 "$0" | tail -21; exit 0 ;; + --specs) + if [[ -z "$2" || "${2:0:1}" == "-" ]]; then + echo -e "${RED}ERROR: --specs requires a non-empty list of spec paths${NC}" + echo "Usage: --specs \"\"" + echo "Example: --specs \"e2e/exam/ExamResults.spec.ts e2e/lecture/\"" + exit 1 + fi + TEST_SPECS="$2" + shift 2 + ;; + --help) head -30 "$0" | tail -26; exit 0 ;; *) echo -e "${RED}Unknown option: $1${NC}"; exit 1 ;; esac done @@ -552,8 +568,17 @@ sample_cpu() { sample_cpu & CPU_MONITOR_PID=$! -# Build base Playwright args -BASE_ARGS=(e2e) +# Build base Playwright args. +# Positional args are the spec paths Playwright runs. Default to the whole e2e/ tree; +# --specs narrows it to an explicit set (word-split on purpose, the option is documented +# as a space-separated list). --grep filters by test title and composes with either. +BASE_ARGS=() +if [ -n "$TEST_SPECS" ]; then + # shellcheck disable=SC2206 # deliberate word splitting: --specs is a space-separated list + BASE_ARGS=($TEST_SPECS) +else + BASE_ARGS=(e2e) +fi if [ -n "$TEST_FILTER" ]; then BASE_ARGS+=(--grep "$TEST_FILTER") fi diff --git a/run-e2e-tests-local-multinode-fast.sh b/run-e2e-tests-local-multinode-fast.sh index 5d466620a966..4a0e323ec8b4 100755 --- a/run-e2e-tests-local-multinode-fast.sh +++ b/run-e2e-tests-local-multinode-fast.sh @@ -29,6 +29,11 @@ set -e # Options: # --stop Tear everything down (host JVMs + infra containers) # --filter Run only tests matching the pattern (e.g., "Quiz") +# --specs "" Run only these spec paths, relative to src/test/playwright +# (e.g., "e2e/exam/ExamResults.spec.ts e2e/lecture/"). +# Replaces the default "run everything under e2e/". +# Combines with --filter. Get the paths for a branch with +# .ci/E2E-tests/determine-relevant-tests.sh # --middleware Distributed data backend: hazelcast (default) or redis. # Both are driven through the DistributedDataProvider # abstraction, so the same tests must pass on either. @@ -52,6 +57,7 @@ SKIP_BUILD=false SKIP_UP=false DEBUG=false TEST_FILTER="" +TEST_SPECS="" # Hazelcast stays the default: it is what production runs today. Redis is the supported alternative and has to pass the # same suite, which is the whole point of the DistributedDataProvider abstraction. MIDDLEWARE="hazelcast" @@ -86,7 +92,17 @@ while [[ $# -gt 0 ]]; do TEST_FILTER="$2" shift 2 ;; - --help) head -40 "$0" | tail -36; exit 0 ;; + --specs) + if [[ -z "$2" || "${2:0:1}" == "-" ]]; then + echo -e "${RED}ERROR: --specs requires a non-empty list of spec paths${NC}" + echo "Usage: --specs \"\"" + echo "Example: --specs \"e2e/exam/ExamResults.spec.ts e2e/lecture/\"" + exit 1 + fi + TEST_SPECS="$2" + shift 2 + ;; + --help) head -46 "$0" | tail -42; exit 0 ;; *) echo -e "${RED}Unknown option: $1${NC}"; exit 1 ;; esac done @@ -721,7 +737,16 @@ pnpm run playwright:setup-local 2>/dev/null rm -f test-reports/results*.xml rm -rf test-reports/monocart-report*/ -BASE_ARGS=(e2e) +# Positional args are the spec paths Playwright runs. Default to the whole e2e/ tree; +# --specs narrows it to an explicit set (word-split on purpose, the option is documented +# as a space-separated list). --grep filters by test title and composes with either. +BASE_ARGS=() +if [ -n "$TEST_SPECS" ]; then + # shellcheck disable=SC2206 # deliberate word splitting: --specs is a space-separated list + BASE_ARGS=($TEST_SPECS) +else + BASE_ARGS=(e2e) +fi if [ -n "$TEST_FILTER" ]; then BASE_ARGS+=(--grep "$TEST_FILTER") fi diff --git a/skills/README.md b/skills/README.md new file mode 100644 index 000000000000..c2c2e3238e52 --- /dev/null +++ b/skills/README.md @@ -0,0 +1,70 @@ +# Artemis agent skills + +Skills that teach an AI coding agent how this repository actually works: which tests a change +needs, why a build is red, and which conventions the build enforces. + +`CLAUDE.md` holds facts and is always in the agent's context. These skills hold procedures and load +only when they are used, which is why they can be long. + +## Installing + +Any agent (Claude Code, Cursor, Codex, Copilot, opencode, Zed, and around seventy others): + +```bash +npx skills add ls1intum/Artemis +``` + +Claude Code, as a versioned plugin with namespaced skills (`/artemis:e2e-pr-check`): + +``` +/plugin marketplace add ls1intum/Artemis +/plugin install artemis@artemis +``` + +## The skills + +| Skill | What it does | +| --------------------- | ------------------------------------------------------------------------------- | +| `e2e-pr-check` | Runs only the Playwright specs a change affects, and reads the result correctly | +| `ci-triage` | Classifies a red build before anyone changes code | +| `server-arch-gates` | Maps a server change to the architectural rules it must satisfy | +| `liquibase-migration` | Writes a changelog that survives a rolling deploy on both databases | +| `client-conventions` | Angular signal APIs, cloning, template control flow, TUM UI styling | +| `write-tests` | Base class selection, and the test commands that silently do the wrong thing | +| `local-setup` | Fresh clone to a running server and client | + +## Contributing a skill + +A skill is a directory under `skills/` containing `SKILL.md`, plus optional `reference/` files that +the body points at. + +```markdown +--- +name: my-skill +description: What it does and when to use it, key use case first. +--- + +# Title + +The procedure. +``` + +Rules for this repository: + +- **Every factual claim cites a repository path.** `supporting_scripts/check_skill_references.py` + runs in CI and fails if a path referenced from `skills/` no longer exists. +- **Keep the body a procedure.** Long background belongs in a `reference/` file, which costs + nothing until the agent reads it. +- **Say why, not just what.** A rule without its reason gets worked around rather than followed. +- **Do not restate `CLAUDE.md`.** Add the part that does not fit there: the steps, the commands, + the failure modes. + +Test a skill before opening a pull request: + +```bash +claude --plugin-dir . # loads the working copy +python3 supporting_scripts/check_skill_references.py +claude plugin validate . +``` + +Documentation for users: `documentation/docs/developer/work-with-ai.mdx`. diff --git a/skills/ci-triage/SKILL.md b/skills/ci-triage/SKILL.md new file mode 100644 index 000000000000..9bc38c9bafae --- /dev/null +++ b/skills/ci-triage/SKILL.md @@ -0,0 +1,70 @@ +--- +name: ci-triage +description: Work out why an Artemis pull request is red before changing any code. Use when CI fails, a check is stuck or missing, a test looks flaky, or someone asks to fix a failing build. Distinguishes real defects from the known infrastructure and harness failures that look identical to them, and gives the correct way to re-run each job. +--- + +# Triage a red Artemis build + +Most red builds on this repository are real. A meaningful minority are not, and the ones that are +not look exactly like the ones that are. Classify first, then debug. Changing code in response to a +harness failure wastes a full CI cycle and adds a confusing commit to the history. + +## Step 1: find out what is actually red + +```bash +gh pr checks +gh run view --log-failed +``` + +The only check that gates a merge is the aggregate **All required CI Passed** +(`.github/workflows/ci.yml`). Individual advisory checks going red does not block anything, so +establish whether the failing job is inside that gate before treating it as urgent. Report PR +Coverage is deliberately outside it. + +The workflows and the jobs they contain: + +| Workflow | Jobs | +| ---------------------------------------------- | -------------------------------------------------------------------------------------------------- | +| `.github/workflows/ci-quality.yml` | Server Code Style, Client Code Style, Client Compilation, Server Code Quality, Query Quality Check | +| `.github/workflows/ci-test.yml` | Server Tests (PostgreSQL), Client Tests | +| `.github/workflows/ci-e2e.yml` | Determine Relevant Tests, Phase 1 and Phase 2 E2E, Report E2E Overall Status | +| `.github/workflows/ci-build.yml` | Build | +| `.github/workflows/ci-bean-instantiations.yml` | Bean instantiation count gate | + +## Step 2: match against the known patterns + +Read `reference/known-failure-patterns.md` before reading the logs as though they describe a +defect. It covers, with the tell for each: + +- Server Tests reporting no failures alongside a timeout +- one ArchUnit violation turning two separate jobs red +- a pull request that starts no CI at all +- checks that never appear after a push +- the counted gates that are at their limit, where an unrelated change trips them +- the genuinely flaky areas of the Server Tests suite + +## Step 3: re-run correctly + +Re-running the wrong way wastes 40 minutes and produces a confusing result. + +- **Server Tests: never use `gh run rerun --failed`.** The suite's sharding and reporting mean a + partial re-run does not reproduce the original conditions. Use a full re-run: + `gh run rerun `. +- **No run started at all**: this is not something a re-run fixes. See the "no CI at all" and + "dropped event" entries in the reference file. +- **A workflow file changed in the branch**: pushing it needs the `workflow` token scope, and the + run uses the workflow definition from the branch, not from develop. + +## Step 4: only now, debug + +Once the failure is classified as real, treat it normally. Reproduce it locally before fixing it: +the local commands for each check are in `skills/server-arch-gates/SKILL.md` for architecture and +style gates, and `skills/write-tests/SKILL.md` for the test suites. + +## What not to do + +- Do not re-run a job repeatedly hoping it goes green. If it is flaky, say so and name the pattern. + If it is not, re-running changes nothing. +- Do not raise a timeout to make a test pass. See `skills/e2e-pr-check/SKILL.md`. +- Do not conclude "flaky" from a single failure. A deterministic failure that only happens in CI is + common in this repository and is usually an environment difference, not chance. diff --git a/skills/ci-triage/reference/known-failure-patterns.md b/skills/ci-triage/reference/known-failure-patterns.md new file mode 100644 index 000000000000..d742f02ca41e --- /dev/null +++ b/skills/ci-triage/reference/known-failure-patterns.md @@ -0,0 +1,116 @@ +# Known CI failure patterns + +Each entry gives the symptom, the tell that distinguishes it from a real defect, and what to do. +If a failure does not match anything here, treat it as real. + +## Server Tests: "Timeout has been exceeded" with no test failures + +**Symptom.** Server Tests (PostgreSQL) is red. The log ends with a Gradle timeout message. Grepping +the log for `FAILED` finds nothing, or finds only unrelated noise. + +**Tell.** The failure count is zero and the job duration is at the backstop. `gradle/test.gradle` +sets a hard `timeout = Duration.ofMinutes(70)` on the test task, inside a job whose own +`timeout-minutes` is 80 (`.github/workflows/ci-test.yml`). The Gradle timeout exists so a hang +fails as a real failure rather than the opaque `cancelled` a job timeout produces. That means a run +that was merely slow, not hung, fails the same way a hang does. + +**What to do.** This is a healthy run killed by the backstop, usually on a loaded runner. Re-run +the whole job. Do not go looking for the failing test, there isn't one. Duration is the reliable +signal here; grepping for failures actively misleads. + +## One ArchUnit violation, two red jobs + +**Symptom.** Both Server Code Style and Server Tests are red. + +**Tell.** Server Code Style runs `./gradlew test -DincludeTags='ArchitectureTest' -x webapp` +(`.github/workflows/ci-quality.yml`), and the same architecture tests also run as part of the full +Server Tests suite. A single violation therefore fails both. + +**What to do.** Fix the one violation. Do not treat the two red jobs as two problems. Reproduce +locally with the architecture-only command above, which takes a fraction of the full suite's time. + +## No CI runs at all + +**Symptom.** The pull request shows no checks. It looks exactly like GitHub dropped the event. + +**Tell.** Check whether the pull request can merge before anything else: + +```bash +gh pr view --json mergeable,mergeStateStatus +``` + +A `CONFLICTING` pull request starts no `pull_request` workflow run at all. + +**What to do.** Resolve the conflicts. Re-running and re-dispatching will not help while the branch +conflicts. Note that a stacked pull request has a base other than develop, so check `baseRefName` +before merging develop into it, or you create exactly this state. + +## Checks never appear after a push + +**Symptom.** A push landed, the branch is not conflicting, and still nothing starts. + +**Tell.** The `synchronize` event was dropped. Nothing in the run list corresponds to the new SHA. + +**What to do.** Dispatch manually: + +```bash +gh workflow run ci.yml --ref +``` + +Note that a `workflow_dispatch` run is not a valid event for every job. Jobs that key off the +pull-request event behave differently or fail under a manual dispatch, so read the result with that +in mind rather than treating a dispatch-only failure as a branch problem. + +## A counted gate trips on an unrelated change + +**Symptom.** A check complains about a count or a threshold rather than about the code that +changed. + +**Tell.** Several gates in this repository count things repository-wide and compare against a +recorded limit that develop already sits at. Adding one more of the counted thing fails the gate no +matter how good the change is: + +- **Large classes.** A class pushed past the size limit fails the gate. Extract a service rather + than raising the number. +- **Bean instantiations** (`.github/workflows/ci-bean-instantiations.yml`). Adding a + `@Configuration` or a new bean can trip it. The failure is reported from a step whose name does + not mention counting, which makes it hard to recognise. +- **Query Quality Check** (`.github/workflows/ci-quality.yml`). A new `@EntityGraph` with more + fetch paths than the baseline allows fails. Reuse an existing counted method where possible; + `supporting_scripts/find_slow_queries.py` is the local check. + +**What to do.** Decide deliberately whether to restructure the change or to raise the threshold, +and say which you chose and why. Raising a limit silently is how these gates stopped being useful +elsewhere. + +## Server Tests flakiness + +**Symptom.** Server Tests fails on a test unrelated to the change, and passes on re-run. + +**Tell.** The suite is genuinely flaky under load on shared runners. Recurring shapes include +sanitizer timeouts in the local CI build path, scheduling-sensitive assertions, and occasional +database process failures. + +**What to do.** A full re-run is the correct response, once. If the same test fails twice, it is +not flakiness. Do not add a retry or extend a timeout to paper over it. + +## A single runner producing impossible git errors + +**Symptom.** Errors such as `bad tree object` that make no sense for the branch, always on the same +runner. + +**Tell.** The failure follows the runner, not the change. Re-running lands on a different runner +and passes. + +**What to do.** This is a corrupted checkout cache on that runner, not a repository problem. Report +the runner so its cache can be purged, and re-run. + +## E2E failures that exist on develop + +**Symptom.** A Playwright spec fails and looks unrelated to the change. + +**Tell.** The same spec also fails on develop. Pull request runs and develop runs do not use the +same topology, so a failure that reproduces only in one of them is expected rather than surprising. + +**What to do.** Compare against develop and against other open pull requests before attributing the +failure to the branch. See `skills/e2e-pr-check/SKILL.md` for how to run a targeted local check. diff --git a/skills/client-conventions/SKILL.md b/skills/client-conventions/SKILL.md new file mode 100644 index 000000000000..7cf074c7c514 --- /dev/null +++ b/skills/client-conventions/SKILL.md @@ -0,0 +1,103 @@ +--- +name: client-conventions +description: Write Angular code for Artemis that passes lint and review the first time. Use when creating or changing anything under src/main/webapp/app or packages/tum-ui, when an ESLint localRules check fails, or when migrating a component to signals. Covers signal APIs, the ngOnChanges ban, template control flow, object cloning, and the TUM UI and Tailwind styling rules. +--- + +# Artemis client conventions + +These are enforced, not advisory. Most have a custom ESLint rule in `rules/` behind them, so +breaking one fails Client Code Style rather than merely attracting a review comment. + +Verify with: + +```bash +pnpm run lint +pnpm run prettier:check +``` + +`reference/migration-recipes.md` has the before-and-after for each migration. Read it when changing +existing code rather than inventing a translation. + +## Signals are mandatory for new code + +Use `input()` / `input.required()`, `output()`, `viewChild()` / `viewChild.required()`, +`viewChildren()`, `signal()`, `computed()`, `effect()`, and `inject()` for dependency injection. + +The legacy decorators `@Input`, `@Output`, `@ViewChild`, `@ViewChildren`, `@ContentChild`, and +`@ContentChildren` must not appear in new code. Enforced by `localRules/enforce-signal-apis` +(`rules/enforce-signal-apis.mjs`) in modules that have been migrated. + +In a module that is not yet fully migrated, prefer signals for new components but stay consistent +within an existing component. Do not half-migrate a component. + +## `ngOnChanges` is banned + +Use `computed()` or `effect()`. Enforced at error level by +`localRules/prefer-signal-reactivity-over-ngonchanges` (`rules/prefer-signal-reactivity-over-ngonchanges.mjs`) +across `src/main/webapp/app`, `packages/tum-ui/src/lib`, and `src/test/javascript`, including specs +and undecorated base classes. + +This is a consistency ban, not a correctness fix. Angular does call inherited `ngOnChanges` hooks +and does fire them for signal inputs, so existing uses are not dead code. + +A genuinely unavoidable case, meaning you need `SimpleChanges.previousValue` or `isFirstChange()`, +or ordering before child initialisation, needs a detailed comment and a justified line-level +`eslint-disable-next-line`. `ngOnInit` and `ngOnDestroy` are unaffected. + +## Template control flow + +Use `@if`, `@for`, `@switch`. Never `*ngIf`, `*ngFor`, `*ngSwitch`. + +## Copying objects + +Use `deepClone` from `src/main/webapp/app/foundation/util/deep-clone.util.ts`. Never object spread, +`Object.assign`, or `structuredClone`, for anything entity-like: anything that may hold a `dayjs` +date, a nested object, a `Map` or `Set`, or a circular reference. + +- `structuredClone()` is the worst option. It does not preserve prototypes, so a cloned `dayjs` + date comes back as a plain object with no methods. +- Spread and `Object.assign` copy one level. Nested objects stay shared, so a later edit mutates + both. + +Two companions live in the same file: `cloneWith(x, { a, b })` replaces `{ ...x, a, b }`, and +`hydrate(new Course(), dto)` replaces `Object.assign(new Course(), dto)` for giving a parsed server +DTO its prototype. + +Enforced by `localRules/prefer-deep-clone` (`rules/prefer-deep-clone.mjs`), production client +TypeScript only, specs exempt. Importing `cloneDeep` from `lodash-es` is blocked so all copying +goes through the wrappers. + +Array spread stays fine: `items.update((items) => [...items, newItem])` is the documented way to +append immutably. Object rest in destructuring is fine too. + +The signal interaction is subtle and is the part people get wrong. See the cloning section of +`reference/migration-recipes.md`. + +## Styling + +Use TUM UI components (`@tumaet/ui-angular`) and Tailwind v4 utilities. Do not add Bootstrap or +ng-bootstrap in new work. + +Colours use semantic tokens. Use TUM UI component variants, or `text-state-danger`, +`text-state-success`, `text-state-warning`, `text-state-info` for plain markup. Never `--p--N` +primitives, never `text-red-500`, never `text-danger`, never the superseded arbitrary +`text-(--danger)` form. Enforced by `localRules/no-raw-tailwind-color-palette` and +`localRules/no-bootstrap-classes`. + +Never hand-write PrimeNG root classes such as `class="p-button"` or `class="p-inputtext"`. Render +the real PrimeNG component so its styles load deterministically. Enforced by +`localRules/no-primeng-component-classes`. + +PrimeNG itself is a transitional fallback, used only when a TUM UI gap cannot reasonably be closed +in the same change. Explain the contained fallback in the pull request. + +If TUM UI lacks a reusable capability, add or evolve a package component around native HTML or +stable Angular CDK primitives, and keep Artemis-specific composition in the application. See +`documentation/docs/developer/guidelines/tum-ui-kit.mdx`. + +## Other rules worth knowing + +Prefer `undefined` over `null`. Aim for full type safety; `localRules/no-as-any-cast` and +`localRules/no-as-unknown-cast` block the usual escape hatches. Filenames are kebab-case. + +Full guidance: `documentation/docs/developer/guidelines/client-development.mdx`. diff --git a/skills/client-conventions/reference/migration-recipes.md b/skills/client-conventions/reference/migration-recipes.md new file mode 100644 index 000000000000..a352f2b3b58c --- /dev/null +++ b/skills/client-conventions/reference/migration-recipes.md @@ -0,0 +1,153 @@ +# Client migration recipes + +Before-and-after for the conversions that come up most, with the reasoning where the mechanical +translation is wrong. + +## `@Input` to `input()` + +```typescript +// before +@Input() course: Course; +@Input() required = false; + +// after +readonly course = input.required(); +readonly required = input(false); +``` + +Reads become calls: `this.course()` rather than `this.course`. In templates, `course()` likewise. + +A two-way binding becomes `model()` rather than an `input()` plus an `output()`. Using the pair +where a `model()` is meant is a common mistake that only shows up when the parent stops receiving +updates. + +## `@Output` to `output()` + +```typescript +// before +@Output() saved = new EventEmitter(); +this.saved.emit(course); + +// after +readonly saved = output(); +this.saved.emit(course); +``` + +## `@ViewChild` to `viewChild()` + +```typescript +// before +@ViewChild('editor') editor: ElementRef; + +// after +readonly editor = viewChild.required('editor'); +``` + +Use `viewChild()` when the child may be absent, `viewChild.required()` when it must exist. + +## Constructor injection to `inject()` + +```typescript +// before +constructor(private courseService: CourseService) {} + +// after +private readonly courseService = inject(CourseService); +``` + +## `ngOnChanges` to `computed()` or `effect()` + +Deriving a value from inputs is a `computed()`: + +```typescript +// before +ngOnChanges() { + this.visibleExercises = this.exercises.filter((e) => e.visible); +} + +// after +readonly visibleExercises = computed(() => this.exercises().filter((e) => e.visible)); +``` + +Reacting with a side effect is an `effect()`. Prefer `computed()` wherever the result is a value: +an `effect()` that only assigns a field is a `computed()` written the hard way. + +Only `SimpleChanges.previousValue`, `isFirstChange()`, and ordering before child initialisation +genuinely need the hook. Those need a comment and a line-level disable. + +## Cloning, and how it interacts with signals + +The rule is `deepClone`, but the interesting part is when to copy at all. + +**Replacing an object in a signal.** A signal notifies only when the reference changes, so replace +rather than mutate: + +```typescript +const updated = deepClone(current); +updated.field = value; +return updated; +``` + +The canonical example is `setImageUrl` in `src/main/webapp/app/core/auth/account.service.ts`. + +**When you only need the signal to emit.** Do not copy at all. Declare the signal with +`equal: () => false` and re-set the same reference. Copying detaches the nested objects that +children already hold, and that ends in `NG0103`. + +**When the state is not signal-backed.** Build the replacement explicitly, field by field, rather +than reaching for a shallow copy. + +**Single-expression copy with overrides.** `cloneWith(x, { a, b })` instead of `{ ...x, a, b }`. The +source is deep-cloned and the overrides are applied by reference. + +**Giving a parsed DTO its prototype.** `hydrate(new Course(), dto)` instead of +`Object.assign(new Course(), dto)`. + +Full rationale with more examples: +`documentation/docs/developer/guidelines/client-development.mdx`. + +## Template control flow + +```angular-html + +
{{ course.title }}
+
  • {{ e.title }}
    • + + +@if (course()) { +
    {{ course()!.title }}
    +} +@for (e of exercises(); track e.id) { +
  • {{ e.title }}
    • +} +``` + +`@for` requires `track`. It is not optional the way `trackBy` was. + +## Colours + +```html + +… +… +… + + +… +``` + +For a component, prefer the TUM UI variant over a utility class on plain markup. + +## Verifying a migration + +```bash +pnpm run lint +pnpm exec vitest run +``` + +A template-only error will not show up in either. Run a production build when a template changed +structurally: + +```bash +pnpm run webapp:prod +``` diff --git a/skills/e2e-pr-check/SKILL.md b/skills/e2e-pr-check/SKILL.md new file mode 100644 index 000000000000..4df0b301df07 --- /dev/null +++ b/skills/e2e-pr-check/SKILL.md @@ -0,0 +1,115 @@ +--- +name: e2e-pr-check +description: Run the Artemis Playwright E2E tests that this branch's changes actually affect, and interpret the result correctly. Use when asked to E2E test a branch or pull request, to check a change end to end before pushing, or to investigate a failing Playwright spec. Covers selecting the affected specs, choosing between the single-node and multi-node runner, and the failure modes that look like real bugs but are not. +--- + +# Run the E2E tests this change affects + +The full Playwright suite is roughly 316 tests and takes tens of minutes. Almost no change needs +all of them. This skill selects the specs the change actually affects, runs them, and then reads +the result with the failure patterns of this suite in mind. + +## Step 1: work out which specs are affected + +Do not guess and do not hand-read `.ci/E2E-tests/e2e-test-mapping.json`. Run the same resolver CI +uses, so local selection and CI selection can never disagree: + +```bash +./.ci/E2E-tests/determine-relevant-tests.sh origin/develop +``` + +It prints five `OUTPUT:` lines. The ones that matter: + +- `RUN_ALL_TESTS=true` means the change hit `runAllTestsPatterns` (Spring config, `docker/`, + `build.gradle`, `angular.json`) or touched Playwright infrastructure outside `e2e/`. Say so + explicitly rather than quietly running a subset. Then either run the full suite or agree with the + user on a narrower scope, but do not present a subset as sufficient coverage. +- `RELEVANT_TESTS` is the space-separated list of spec paths to run, relative to + `src/test/playwright`. It always includes the always-run specs (`e2e/Login.spec.ts`, + `e2e/Logout.spec.ts`, `e2e/SystemHealth.spec.ts`). +- `REMAINING_TESTS` is everything else. CI runs it as phase 2 only after phase 1 passes. Locally it + is normally not worth running. + +Two things about the input: + +- **It diffs commits, not the working tree.** The script runs `git diff --name-only ...HEAD`, + so uncommitted changes are invisible to it and it reports "No changed files detected. Running all + tests." Commit first, or the selection will be wrong in the direction of running everything. +- **Pass a different base for a stacked branch.** The base is the first argument. A stacked pull + request is not cut from develop, so diffing against develop selects its parent's changes too. + +## Step 2: choose the runner + +Default to the single-node runner. It is faster and it is what most changes need. + +```bash +./run-e2e-tests-local-fast.sh --specs "" +``` + +Use the multi-node runner instead when the diff touches cluster-sensitive code, because a single +node cannot reproduce cross-node failures at all: + +```bash +./run-e2e-tests-local-multinode-fast.sh --specs "" +``` + +Treat a change as cluster-sensitive when it touches any of: + +- `src/main/java/de/tum/cit/aet/artemis/core/service/distributed/` or any caller of + `DistributedDataProvider` +- `src/main/java/de/tum/cit/aet/artemis/core/config/cache/` +- the build job queue and dispatch in `src/main/java/de/tum/cit/aet/artemis/localci/` +- websocket broker or scheduling configuration + +If the change is specifically about the distributed data abstraction, run the suite on both +backends. Redis has to pass the same tests as Hazelcast, and with `--middleware redis` no Hazelcast +instance is created at all, which is what makes it a genuine test of the abstraction: + +```bash +./run-e2e-tests-local-multinode-fast.sh --middleware redis --specs "" +``` + +## Step 3: re-runs + +The runners keep services alive between runs. After the first run, reuse them: + +```bash +./run-e2e-tests-local-fast.sh --skip-server --skip-client --skip-db --specs "" +``` + +For the multi-node runner the equivalent is `--skip-build --skip-up`. + +Tear down with `--stop` when finished. Leaving services running is fine and normal during +iteration, but see the wrong-client trap below. + +## Step 4: interpret the result + +A red spec is not automatically a bug in the change. Work through these before concluding anything. + +**Does it already fail on develop?** Some specs fail only in develop's full-suite job, so a green +run on a pull request proves less than it looks and a red one may be pre-existing. Check the same +spec on develop before attributing the failure to the branch. + +**Is the client under test actually this branch?** The runners reuse whatever is serving ports 9000 +and 8080. A dev server left running from another branch will serve different code and the failure +will make no sense. If a failure looks impossible, verify what is actually being served before +debugging further. + +**Is the assertion counting shared state?** Tests run across parallel workers against one server, +so anything asserting on a server-wide counter must assert a lower bound, not an exact value or an +exact delta. A single-worker local run hides this class of bug entirely, so a test that passes +locally and fails in CI with an off-by-a-few count is usually this. + +**Never fix a flake by raising a timeout.** Raising a timeout hides the race rather than fixing it, +and the test stays flaky in CI where the machine is slower and more loaded. Find what the test is +actually waiting for. + +**Did the run produce zero tests?** `--specs` paths are relative to `src/test/playwright`, and a +typo yields "no tests found" rather than an error. Check the count in the summary matches what you +expected from step 1. + +## Reporting back + +State which specs ran, which passed, and which failed. If step 1 reported `RUN_ALL_TESTS=true` and +a subset was run anyway, say so plainly. Do not describe a change as E2E tested when the selection +was narrowed for time. diff --git a/skills/liquibase-migration/SKILL.md b/skills/liquibase-migration/SKILL.md new file mode 100644 index 000000000000..199ff3c03638 --- /dev/null +++ b/skills/liquibase-migration/SKILL.md @@ -0,0 +1,73 @@ +--- +name: liquibase-migration +description: Write an Artemis Liquibase changelog that survives a rolling deployment on both PostgreSQL and MySQL. Use when adding, changing, or dropping a database column, table, index, or constraint, or when a changeset fails on startup. Covers the file and id conventions, the guarded pattern for adding NOT NULL, expand and contract for rolling deploys, and the local validation steps. +--- + +# Write a Liquibase migration + +A bad changeset does not fail a test, it stops the application from starting, on every node, in +production. Everything here exists because of that. + +## The mechanics + +Changelogs live in `src/main/resources/config/liquibase/changelog/` and are included from +`src/main/resources/config/liquibase/master.xml`. + +1. Get the timestamp: `date '+%Y%m%d%H%M%S'` +2. Create `src/main/resources/config/liquibase/changelog/_changelog.xml` +3. Add an `` line for it at the end of `master.xml`, keeping chronological order + +Changeset ids are `--`, for example +`20260827090000-02-result-submission-not-null`. The author is your username. Never edit a changeset +that has already been merged: Liquibase records a checksum and the application refuses to start +when it changes. Write a new changeset instead. + +Read `reference/migration-patterns.md` for the worked patterns. The rest of this file is the +decision procedure. + +## Which pattern do you need? + +**Adding a nullable column, a table, or an index.** Straightforward. Write the changeset, add a +`` if Liquibase cannot infer one. + +**Adding a NOT NULL constraint to an existing column.** Use the guarded pattern. Adding the +constraint while a null is still present fails the changeset, and a failing changeset stops the +application from starting. This is the single most dangerous migration in this codebase and the +pattern is non-obvious, so read the section in `reference/migration-patterns.md` before writing it. + +**Dropping or renaming a column that code still reads.** Use expand and contract across two +releases. During a rolling deployment, nodes on the old version are still running. + +**Anything involving a trigger or a stored routine.** Do not. This repository removed its last +trigger when it moved to PostgreSQL and has rejected proposals to add new ones. Express the +behaviour in the entity design or in application code instead. + +## Both databases + +Artemis runs on PostgreSQL and MySQL. CI tests PostgreSQL. Production configuration hardcodes the +PostgreSQL dialect with no probing, so a MySQL deployment must override `spring.jpa.database`. + +Consequences when writing a changeset: + +- Prefer Liquibase's own change types over ``. They generate correct SQL for both. +- Where you must write raw SQL, check it against both dialects, or split it with a `dbms` attribute. +- CI will not catch a MySQL-only break. If a changeset contains raw SQL, validate it locally + against MySQL. `reference/migration-patterns.md` has the procedure. + +## Verify before pushing + +Start the application against a database that already has data, not an empty one. An empty database +makes every backfill and every precondition trivially pass, which is exactly the case that is never +interesting. + +```bash +./gradlew bootRun -x webapp +``` + +Watch the startup log for the changeset ids. A changeset skipped by a precondition logs a warning +rather than failing, so a silent skip is easy to miss. + +## Related + +Entity-side rules, including why adding a NOT NULL to a column held by a cascading collection +fails, are in `skills/server-arch-gates/SKILL.md`. diff --git a/skills/liquibase-migration/reference/migration-patterns.md b/skills/liquibase-migration/reference/migration-patterns.md new file mode 100644 index 000000000000..4f7b70431735 --- /dev/null +++ b/skills/liquibase-migration/reference/migration-patterns.md @@ -0,0 +1,110 @@ +# Liquibase migration patterns + +Worked patterns with the reasoning. Real examples from this repository are cited so you can read +the full changeset rather than a fragment. + +## Adding a NOT NULL constraint to an existing column + +The problem: `addNotNullConstraint` fails if a single row still holds a null, and a failing +changeset stops the application from starting. On a production database you cannot know in advance +that no such row exists. + +The pattern is two changesets per column: one that clears the rows without a parent, and one that +adds the constraint behind a precondition. + +```xml + + Remove results that belong to no submission, along with everything that hangs off them. + + DELETE FROM feedback WHERE result_id IN (SELECT id FROM result WHERE submission_id IS NULL); + DELETE FROM result WHERE submission_id IS NULL; + + + + + SELECT COUNT(*) FROM result WHERE submission_id IS NULL + + + +``` + +Three things about this are deliberate and easy to get wrong. + +**`onFail="CONTINUE"`, never `MARK_RAN`.** `CONTINUE` skips the changeset without recording it in +`databasechangelog`, so the server starts, a warning is logged, and the constraint is attempted +again on the next startup. Once the offending rows are gone the column constrains itself. +`MARK_RAN` would record the changeset as done, and that installation would keep a nullable column +for good, fixable only by a new changelog. + +**One pair of changesets per column.** Keeping them separate means a single column can be dropped +from the migration without disturbing the others. + +**Only delete rows whose own dependants you can clear.** Where a row is reachable through several +other tables, deleting it in a changelog is the wrong place; that belongs in the relevant deletion +service. Leave the column nullable and say why in the comment. + +Full example, including the reasoning for each column that was deliberately left out: +`src/main/resources/config/liquibase/changelog/20260827090000_changelog.xml`. + +### When the entity mapping blocks it + +Some columns cannot be made NOT NULL without an application change first. If the column is the +owning side of an association held by the parent in a `mappedBy` collection that cascades, +Hibernate inserts the child before it writes the key and fills it in with a later update. That is +invisible while the column allows null and fails immediately once it does not. + +Making such a column NOT NULL means giving every place that adds to the collection the back +reference first, in the `add*` helper on the parent. Roughly fifty owning associations in this +codebase sit behind a cascading collection, so check the mapping before assuming a column is a +simple case. + +## Expand and contract + +For dropping or renaming a column that code still reads, split the work across two releases, +because during a rolling deployment nodes on the previous version are still serving traffic. + +1. **Expand.** Create the new structure, backfill it, and move every reader and writer across. + Leave the old column in place, still populated. +2. **Contract.** Once the expand release is fully deployed, stop mapping the old column, then drop + it in a later release. + +The contract changelog must state that it must not be deployed before its predecessor. + +Rollback deserves thought here. Re-creating a dropped column without its values is often worse than +having no rollback, because code would silently read empty columns. Where the data now lives +elsewhere, say so in the rollback comment and rely on rolling the predecessor back instead. + +Worked example: `src/main/resources/config/liquibase/changelog/20260826080000_changelog.xml`. + +## Rollbacks + +Liquibase infers a rollback for most of its own change types. Write an explicit `` when: + +- the change is raw `` +- the change drops something, so the inferred rollback would be wrong or impossible +- the inferred rollback would restore structure without data + +## Validating against MySQL locally + +CI runs PostgreSQL. A MySQL-only break therefore reaches production undetected. Validate locally +whenever a changeset contains raw SQL or a type whose spelling differs between the two. + +The approach that works: seed a MySQL database using a checkout of develop, so it has the schema as +it exists before your change, then start your branch against that same database and let Liquibase +migrate it. Compare the resulting schema against one built from scratch. A migration that produces +a different schema than a fresh install is a bug, and it is the class of bug this check exists to +find. + +Remember that a MySQL deployment needs `spring.jpa.database` overridden, since the production +configuration hardcodes the PostgreSQL dialect. + +## Things that are not allowed + +**Triggers and stored routines.** The last trigger was removed when PostgreSQL support arrived, and +proposals to add new ones have been rejected. Express it in the entity design instead. + +**Editing a merged changeset.** Liquibase stores a checksum. Changing the file makes every existing +installation refuse to start. Add a new changeset. + +**Assuming an empty database.** Test against data. Every precondition and backfill passes trivially +on an empty schema, which is the one case that never matters. diff --git a/skills/local-setup/SKILL.md b/skills/local-setup/SKILL.md new file mode 100644 index 000000000000..404bbd971c65 --- /dev/null +++ b/skills/local-setup/SKILL.md @@ -0,0 +1,101 @@ +--- +name: local-setup +description: Get a local Artemis development environment running from a fresh clone, or fix one that has stopped working. Use when setting up the project for the first time, when the server or client will not start, when Gradle or pnpm complain about versions, or when unsure which command to run for server-only versus full-stack development. Covers prerequisites, the two run modes, test users, and mail capture. +--- + +# Get Artemis running locally + +## Prerequisites + +| Tool | Version | Note | +| ------ | ---------------- | --------------------------------------------------------------------- | +| JDK | 25 | Pinned by the Gradle toolchain | +| Node | 24.20.0 or newer | Pinned in `gradle.properties` and `package.json` | +| pnpm | 11.25.0 | Pinned by the `packageManager` field; activate with `corepack enable` | +| Docker | current | Required for the database and for server tests | + +Run `corepack enable` once. It activates the exact pnpm version the repository pins, which avoids a +whole category of lockfile arguments. + +On macOS, Homebrew's `openjdk@25` is keg-only, so it is not on the path after installation. Symlink +it rather than exporting `JAVA_HOME` in each shell; a permanent symlink means Gradle finds it +without a per-command prefix. + +## Install dependencies + +```bash +corepack enable +pnpm install --frozen-lockfile +``` + +Use `--frozen-lockfile` unless you are deliberately changing dependencies, in which case plain +`pnpm install` lets the lockfile update. + +## Two ways to run + +**Full stack in one command.** Slower to restart, fine for server work where the client rarely +changes: + +```bash +./gradlew bootRun +``` + +**Server and client separately.** This is what you want for client work, because the Angular dev +server does hot module replacement: + +```bash +./gradlew bootRun -x webapp # terminal 1: server only +pnpm start # terminal 2: Angular dev server with HMR +``` + +The client is then on port 9000 and the server on 8080. + +Expect roughly thirty seconds of startup. That is the normal cold start, not a symptom. Disabling +feature modules barely changes it, because most of it is Spring context work that lazy +initialisation already defers. + +## Test users + +The E2E tooling creates the Playwright test users: + +```bash +supporting_scripts/create_test_users.sh +``` + +The fast E2E runner does this as part of its setup, so if you have run +`./run-e2e-tests-local-fast.sh` you already have them. + +## Seeing outgoing mail + +Artemis only sends mail when it is configured to. To view what it would send, run a local Mailpit +alongside the server and point the mail configuration at it. See +`documentation/docs/developer/mailpit-setup.mdx`. + +## When it will not start + +**"Unable to determine Dialect".** The Spring profile set does not include a database profile, or +an `autoconfigure.exclude` is replacing rather than merging the expected exclusions. + +**The server logs "Started ArtemisApp" but then shuts down.** A Spring Boot and Spring Cloud version +mismatch does exactly this. The two are coupled: a Boot minor bump needs the matching Cloud release +train. Both are pinned in `gradle.properties`. + +**Aggregate health reports DOWN.** This does not by itself mean the server is broken. Check the +readiness and liveness endpoints and look for "Started ArtemisApp" in the log; a single unconfigured +optional integration pulls the aggregate down. + +**Port already in use.** The E2E runner kills processes on 8080, 9000, and 7921 before starting. +`./run-e2e-tests-local-fast.sh --stop` is a quick way to clear all three. + +## Running things + +```bash +./gradlew test -x webapp # server tests, needs Docker +pnpm run vitest # client tests, watch mode +./run-e2e-tests-local-fast.sh # E2E, brings up everything it needs +pnpm run lint # client lint +./gradlew spotlessApply # fix Java formatting +``` + +Full setup documentation, including IDE configuration and the optional integrations: +`documentation/docs/developer/setup.mdx`. diff --git a/skills/server-arch-gates/SKILL.md b/skills/server-arch-gates/SKILL.md new file mode 100644 index 000000000000..118bf9d78d98 --- /dev/null +++ b/skills/server-arch-gates/SKILL.md @@ -0,0 +1,85 @@ +--- +name: server-arch-gates +description: Check Artemis server code against the architectural rules the build enforces, before pushing. Use when writing or changing Java under src/main/java, adding a service, repository, REST resource, DTO, cache, or cross-node state, or when an ArchUnit test fails and the message does not make the rule obvious. Gives the rule, the reason, and the exact local command that proves it. +--- + +# Server architecture gates + +Artemis enforces its server conventions with roughly 174 ArchUnit test classes under +`src/test/java`. They are not style preferences. Each one exists because the pattern it forbids +produced a production bug. The failure messages are often terse, so this skill maps a change to the +rules it is subject to and to the reason behind each. + +## Run them locally + +The whole architecture suite, which is what the Server Code Style job runs: + +```bash +./gradlew test -DincludeTags='ArchitectureTest' -x webapp +``` + +This is much faster than the full server test suite. Run it before pushing any change to +`src/main/java`. A violation fails both Server Code Style and Server Tests, so it is worth catching +locally. + +A single class while iterating: + +```bash +./gradlew test --tests ArchitectureTest -x webapp +``` + +## Which rules apply to what you changed + +| You changed | Read | +| -------------------------------------- | --------------------------------------------------- | +| A service or REST resource | Transactions, persistence access, module boundaries | +| A repository | Transactions, raw JDBC | +| A DTO record | DTO conventions | +| Anything holding state across requests | Caching, distributed data | +| An entity or an association | Caching, entity conventions | +| Anything at all in a large file | Counted gates | + +The detail for each, with the reason and the failing rule name, is in `reference/gates.md`. Read +it rather than guessing; several of these rules forbid something that looks completely reasonable. + +## The rules most often broken + +**No transaction boundaries in services or controllers.** `@Transactional`, +`TransactionTemplate`, and `PlatformTransactionManager` belong in repositories, typically on +modifying queries. Enforced by `testTransactional` in +`src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleRepositoryArchitectureTest.java`. + +**No direct persistence access.** No injected `EntityManager` or `EntityManagerFactory`, and no +`JdbcClient`, `JdbcTemplate`, or `DataSource`. Write the statement as a `@Query` on a repository, +with `nativeQuery = true` where there is no entity to name. Enforced by +`shouldNotUseEntityManagerDirectly` and `shouldNotUseRawJdbcDirectly` in +`src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java`. + +**Never touch Hazelcast or Redis directly.** All cross-node state goes through +`DistributedDataProvider` in +`src/main/java/de/tum/cit/aet/artemis/core/service/distributed/`. Enforced by +`src/test/java/de/tum/cit/aet/artemis/shared/architecture/DistributedDataProviderArchitectureTest.java`. +The backend is configurable, so direct usage does not fail loudly, it silently loses the state. + +**No Hibernate second-level cache.** No `@Cache` on entities or associations. Enforced by +`testNoHibernateSecondLevelCacheAnnotation` in `ArchitectureTest.java`. For DTO and projection +caching use Spring `@Cacheable`, always paired with explicit eviction. + +**Reach optional modules through their API.** Use `Optional<*Api>`, never another module's +repository directly. + +## Before adding a cache + +The default answer is not to. The bar is a measured performance gain that justifies the +eviction-correctness work, because there is no service-level transaction boundary to coordinate +eviction within a request. See `documentation/docs/developer/guidelines/caching.mdx` for the full +rationale, and `reference/gates.md` for the pattern if you do proceed. + +## Adding a capability to the distributed data layer + +If `DistributedDataProvider` lacks what you need, add it there, implement it for all three backends +(Hazelcast, Redis, Local), and add a case to `AbstractDistributedDataTest`. That suite is what keeps +the backends in agreement. Request entry lifetimes at the call site with +`getExpiringMap(name, ttl)`; `getMap(name)` rejects a per-entry TTL deliberately, because a backend +map configuration only applies to that one backend. Full guidance: +`documentation/docs/developer/guidelines/distributed-data.mdx`. diff --git a/skills/server-arch-gates/reference/gates.md b/skills/server-arch-gates/reference/gates.md new file mode 100644 index 000000000000..4c4661dcf161 --- /dev/null +++ b/skills/server-arch-gates/reference/gates.md @@ -0,0 +1,125 @@ +# Server architecture gates in detail + +Every rule here is enforced by a test. The reason matters as much as the rule: several of these +forbid something that looks perfectly reasonable, and knowing why stops you working around the +check instead of the problem. + +## Transactions + +**Rule.** No `@Transactional`, `TransactionTemplate`, or `PlatformTransactionManager` in services or +controllers. Transaction boundaries may only be defined inside repositories, typically for +modifying queries. + +**Enforced by.** `testTransactional` in +`src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleRepositoryArchitectureTest.java`, +which each module's `*RepositoryArchitectureTest` subclass runs. + +**Consequence for you.** A REST call is not one transaction. Do not write code that assumes reads +later in the call see writes from earlier in the call rolled into one atomic unit, and do not +attempt to coordinate cache eviction across a request boundary that does not exist. + +## Persistence access + +**Rule.** No injected `EntityManager` or `EntityManagerFactory`. No `JdbcClient`, `JdbcTemplate`, or +`DataSource`. All persistence goes through Spring Data repositories. Where there is no entity to +name, write a `@Query` with `nativeQuery = true`. + +**Enforced by.** `shouldNotUseEntityManagerDirectly` and `shouldNotUseRawJdbcDirectly` in +`src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java`. The raw JDBC rule +permits `core.config` only. + +## Distributed data + +**Rule.** Never use Hazelcast or Redis directly. Everything crossing a node boundary, including the +build job queue, feature toggles, scheduling messages, websocket broker status, LTI state, Pyris +jobs, and `@Cacheable` caches, goes through `DistributedDataProvider` in +`src/main/java/de/tum/cit/aet/artemis/core/service/distributed/`. + +**Enforced by.** +`src/test/java/de/tum/cit/aet/artemis/shared/architecture/DistributedDataProviderArchitectureTest.java`, +which fails the build if a production class outside a small named set of backend adapters depends +on `com.hazelcast..`, `org.redisson..`, or `org.springframework.data.redis..`. + +**Why it is not merely stylistic.** The backend is selected by `artemis.distributed-data.provider`. +With the Redis backend, no Hazelcast instance is created at all, so a direct Hazelcast call does not +throw. It silently writes state nowhere. + +**Adding a capability.** Add it to `DistributedDataProvider`, implement it for Hazelcast, Redis, and +Local, and add a case to +`src/test/java/de/tum/cit/aet/artemis/core/service/distributed/AbstractDistributedDataTest.java`. +That suite is the only thing keeping the three backends in agreement. + +**Entry lifetimes.** Request them at the call site with `getExpiringMap(name, ttl)`. `getMap(name)` +rejects a per-entry TTL on purpose: a backend map configuration only applies to that backend, so a +TTL configured there would silently not apply under a different provider. + +Full guidance: `documentation/docs/developer/guidelines/distributed-data.mdx`. + +## Caching + +**Rule.** No `@Cache` (Hibernate second-level) annotations on entities or associations. + +**Enforced by.** `testNoHibernateSecondLevelCacheAnnotation` in `ArchitectureTest.java`. + +**Why.** The second-level cache is disabled cluster-wide. `@Modifying @Query` repository methods +bypass its invalidation, and because there is no service-level `@Transactional` there is no clean +place to coordinate eviction within a REST call. Both produced cross-node stale-read bugs. + +**What to use instead.** Spring `@Cacheable`, which resolves against the `RoutingCacheManager` in +`src/main/java/de/tum/cit/aet/artemis/core/config/cache/CacheManagerConfiguration.java`. That serves +blob caches (`files`, `plantUmlPng`, `plantUmlSvg`) from a bounded per-node Caffeine cache and +everything else from the distributed data provider. + +**Always pair it with explicit eviction.** Either `@CacheEvict` on the writing service, or a +Hibernate `PostUpdateEventListener` / `PostDeleteEventListener`. The canonical patterns are +`src/main/java/de/tum/cit/aet/artemis/core/service/TitleCacheEvictionService.java` and, for evicting +a per-node blob cache across the cluster, +`src/main/java/de/tum/cit/aet/artemis/core/service/cache/BlobCacheEvictionService.java`. + +**The bar.** A measured performance gain that justifies the eviction-correctness work. The default +answer is: do not cache. Full rationale and history: +`documentation/docs/developer/guidelines/caching.mdx`. + +## DTOs + +**Rule.** REST endpoints use DTOs, written as Java records. DTOs in a `dto` package need +`@JsonInclude(JsonInclude.Include.NON_EMPTY)`, not `NON_NULL`, and a name ending in `DTO`. + +**Enforced by.** `testJsonIncludeNonEmpty` and `testNoClassFieldsInDtos` in `ArchitectureTest.java`. + +**Two things that surprise people.** The rule scans test classes that live in `dto` packages too, +so a test helper placed there must also satisfy the naming rule. And the architecture thresholds +count violations, not DTOs, so the number in the test does not correspond to a DTO count. + +## Module boundaries + +**Rule.** Modules are packages within one source set, enforced only by ArchUnit. Reach an optional +module through `Optional<*Api>`, never through its repository. + +**Watch out for.** Module-scoped rules only run where the corresponding `*ArchitectureTest` +subclass exists. Some modules have no subclass for some rule families, so the absence of a failure +does not prove compliance. There is also an ignore list that can hide an optional-module repository +leak. + +## Counted gates + +These compare a repository-wide count against a recorded limit that develop already sits at, so an +otherwise good change can fail them. + +**Large classes.** `supporting_scripts/analyze_java_files.py` flags classes over 1000 lines and +compares the count against a maximum. Touching an already-oversized class can push the count up. +Extract a service rather than raising the limit. + +**Bean instantiations.** `.github/workflows/ci-bean-instantiations.yml`. Adding a `@Configuration` +or a bean can trip it, and the failure surfaces in a step whose name does not mention counting. + +**Query quality.** A new `@EntityGraph` with more fetch paths than the baseline allows fails the +Query Quality Check job in `.github/workflows/ci-quality.yml`. Reuse an existing counted method +where you can. Local check: `supporting_scripts/find_slow_queries.py`. + +## Database + +**No triggers and no stored routines.** The entity design is the place to express this instead. + +**Adding a NOT NULL column to an existing table** needs the guarded migration pattern. See +`skills/liquibase-migration/SKILL.md`. diff --git a/skills/write-tests/SKILL.md b/skills/write-tests/SKILL.md new file mode 100644 index 000000000000..1599ebb6d181 --- /dev/null +++ b/skills/write-tests/SKILL.md @@ -0,0 +1,68 @@ +--- +name: write-tests +description: Write an Artemis server or client test that passes on the first CI run. Use when adding or changing a JUnit test under src/test/java or a Vitest spec under src/main/webapp, when a test passes locally but fails in CI, or when unsure which base class or test command to use. Covers base class selection, the admin naming rule, date comparisons, and the Vitest invocations that silently do the wrong thing. +--- + +# Write tests that pass first time + +Most of the friction in this repository's test suites comes from a handful of specific traps, not +from testing being hard. This skill is the list. + +## Server tests + +Server tests need Docker. They run against PostgreSQL through Testcontainers, locally and in CI. + +```bash +./gradlew test -x webapp # everything +./gradlew test --tests ExamIntegrationTest -x webapp # one class +./gradlew test --tests ExamIntegrationTest.testGetExamScore # one method +./gradlew test -DincludeTags='ArchitectureTest' -x webapp # architecture only, fast +``` + +Name tests `*Test.java`. Reuse the module's base class where one exists. + +Read `reference/server.md` for base class selection, the admin naming rule that forces a different +`@ResourceLock`, date comparison, and shared-spy flakiness. + +The one to know before you start: **in the admin module, naming a test `*IntegrationTest` forces it +onto a batch base class carrying a shared `@ResourceLock`.** A test that mutates global state and +needs isolation must be named `*Test` and extend `AbstractSpringIntegrationIndependentTest` +instead. This is enforced by +`src/test/java/de/tum/cit/aet/artemis/admin/architecture/AdminTestArchitectureTest.java`, so getting +it wrong fails the architecture gate rather than the test. + +## Client tests + +Vitest, not Jest. Use `vi.spyOn()`, `vi.fn()`, `vi.clearAllMocks()`. + +```bash +pnpm run vitest # watch +pnpm run vitest:run # single run, everything +pnpm exec vitest run # single file +pnpm run vitest:coverage +pnpm run test-diff # only specs affected by the diff +``` + +**`pnpm run vitest:run -- ` runs the entire suite.** The path is swallowed. Use +`pnpm exec vitest run ` for a single file. This wastes a lot of time before people notice. + +**Vitest is not the type check CI runs.** CI runs a stricter spec `tsc`: + +```bash +pnpm run compile:tests +``` + +It enforces member visibility, which Vitest does not. A spec that reaches a private member as +`component.privateThing` passes locally and fails in CI. Use bracket access, +`component['privateThing']`, and run `compile:tests` before pushing. + +Read `reference/client.md` for the rest: the monaco stub, zoneless test setup, `model()` versus +`input()` plus `output()`, and why template errors need a build rather than a test run. + +## Both + +Keep tests deterministic. Mock external services and WebSockets. CI enforces per-module coverage +thresholds, so a new class with no test can fail the build even when nothing is broken. + +For E2E tests, see `skills/e2e-pr-check/SKILL.md`. Do not add a Playwright test for something a +unit or integration test can cover; the E2E suite is the slowest feedback loop in the project. diff --git a/skills/write-tests/reference/client.md b/skills/write-tests/reference/client.md new file mode 100644 index 000000000000..d906bf27d66f --- /dev/null +++ b/skills/write-tests/reference/client.md @@ -0,0 +1,75 @@ +# Client test reference + +## Invocation + +```bash +pnpm exec vitest run # a single file +pnpm run vitest # watch mode +pnpm run vitest:run # the whole suite +pnpm run test-diff # only specs affected by the diff against develop +pnpm run compile:tests # the strict spec tsc that CI runs +``` + +`pnpm run vitest:run -- ` does **not** filter. The argument is swallowed and the whole suite +runs. + +## The type check CI runs is stricter than Vitest + +`pnpm run compile:tests` type-checks against `tsconfig.spec.json` and enforces member visibility. +Vitest does not. A spec reaching a private member directly compiles under Vitest and fails in CI. + +```typescript +// fails compile:tests +expect(component.privateHelper).toBeDefined(); + +// passes both +expect(component['privateHelper']).toBeDefined(); +``` + +Run `compile:tests` before pushing any spec change. + +## Signal inputs in specs + +A component using `input()` is driven in a spec through the component ref, not by assigning a +field. A `model()` is a two-way binding: replacing it with an `input()` plus an `output()` makes the +parent stop receiving updates, and the spec will not necessarily catch that. + +`MockProvider` does not stub a signal that a service initialises as a field. If a component reads a +shared signal from a service, provide the real service or an explicit stub object; a `MockProvider` +gives back `undefined` and the failure is confusing. + +## Zoneless change detection + +The client runs zoneless. A plain field that gates an `@if` will not trigger a re-render when it +changes after an async load: the template never re-evaluates. Make the guard a `signal()`. + +This is the most common cause of a component that renders correctly in the browser during +development but shows an empty template in a spec after an awaited call, or the reverse. + +## Monaco + +Monaco is stubbed under Vitest. A spec cannot exercise real editor behaviour. There is a separate +configuration, `vitest.monaco.config.ts`, run by `pnpm run vitest:monaco`, for the specs that need +the real thing. + +## Template errors + +Neither Vitest nor `compile:tests` catches every template error. A structurally changed template +needs a real build: + +```bash +pnpm run webapp:prod +``` + +## Stray compiled JavaScript + +If a large number of specs suddenly fail with errors about reading a property of `undefined` on +what should be an enum, look for compiled `.js` files sitting next to their `.ts` sources. They are +gitignored, so they are invisible in `git status`, and they shadow the TypeScript. CI is unaffected, +which makes it look like a local-only mystery. Delete them. + +## Committing + +A pre-commit hook formats staged files. Do not commit while a background process is still editing +the tree, or the hook will format a half-written state. Verify what was committed by reading the +content, not by trusting an exit code. diff --git a/skills/write-tests/reference/server.md b/skills/write-tests/reference/server.md new file mode 100644 index 000000000000..043c0562f755 --- /dev/null +++ b/skills/write-tests/reference/server.md @@ -0,0 +1,68 @@ +# Server test reference + +## Choosing a base class + +The bases live in `src/test/java/de/tum/cit/aet/artemis/shared/base/`: + +| Base | Use for | +| ----------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | +| `AbstractSpringIntegrationIndependentTest` | The default. A test that needs the Spring context but no CI or version control backend. | +| `AbstractSpringIntegrationIndependentBatchTest` | Same context, but batched under a shared `@ResourceLock`, so tests using it never run concurrently with each other. | +| `AbstractSpringIntegrationLocalCILocalVCTest` | Needs the embedded git server and local CI. | +| `AbstractSpringIntegrationJenkinsLocalVCTest` | Needs the Jenkins connector. | + +Each module's `*TestArchitectureTest` declares which bases its integration tests may extend, so the +choice is not free. Look at the module's own architecture test before picking one. + +## The admin naming rule + +`AbstractModuleTestArchitectureTest.integrationTestsShouldExtendAbstractModuleIntegrationTest` +requires every class in a module whose name ends in `IntegrationTest` to extend one of the bases +that module declares. + +In the admin module those are `AbstractSpringIntegrationIndependentBatchTest` and +`AbstractSpringIntegrationLocalCILocalVCTest` +(`src/test/java/de/tum/cit/aet/artemis/admin/architecture/AdminTestArchitectureTest.java`). The +batch base carries `@ResourceLock("AbstractSpringIntegrationIndependentBatchTest")`. + +The consequence: a test that mutates global state and needs to run in isolation cannot be called +`*IntegrationTest` in that module, because the name alone puts it in a shared lock group with every +other batch test. Name it `*Test` and extend `AbstractSpringIntegrationIndependentTest`. + +The failure mode if you get this wrong is not a clear message. It is either an architecture test +failure naming a base class, or intermittent cross-test interference. + +## Dates + +PostgreSQL stores timestamps as UTC and does not preserve the offset through a round-trip. Compare +`ZonedDateTime` values with `toInstant()`: + +```java +assertThat(actual.getDueDate().toInstant()).isEqualTo(expected.getDueDate().toInstant()); +``` + +Comparing the `ZonedDateTime` values directly passes locally in one timezone and fails in another. + +## Shared spies and background threads + +A `UnfinishedStubbingException` raised inside `@BeforeEach` means a shared spy was touched by a +background `@Async` thread while the test was setting up its stubs. + +The fix is in the background path, not in the test. Making the test wait longer, or re-ordering the +stubbing, moves the race rather than removing it. Find what is still running from the previous test +and stop it deterministically. + +## Coverage + +CI enforces coverage thresholds per module. A new class with no test can fail the build even when +every existing test passes. Check the module's threshold before assuming a change needs no test. + +## Running the suite while working + +Do not run a build in the same worktree while the full suite is running. A concurrent +`spotlessApply` or `compileJava` invalidates the running build's classpath and produces scattered +failures that look like real ones but start mid-run. + +For the same reason, a `clean` from a second Gradle invocation wipes the test classpath underneath +an in-flight run. If failures start appearing partway through a previously healthy run, suspect a +concurrent build before suspecting the code. diff --git a/supporting_scripts/check_skill_references.py b/supporting_scripts/check_skill_references.py new file mode 100644 index 000000000000..a991d6333c9b --- /dev/null +++ b/supporting_scripts/check_skill_references.py @@ -0,0 +1,113 @@ +#!/usr/bin/env python3 +"""Verify that every repository path cited by an agent skill still exists. + +Skills under skills/ describe procedures in terms of concrete files: base classes, ArchUnit tests, +runner scripts, workflows. A skill that names a file which has since moved is worse than no skill, +because it is confidently wrong and an agent will act on it. This check is what keeps that from +happening silently. + +It scans every file under skills/ for backtick-quoted tokens that look like repository-relative +paths, resolves each against the repository root, and fails listing the ones that do not exist. +Code blocks are included on purpose: an example command naming a stale path is exactly the case +that does the most damage. + +Usage: + python3 supporting_scripts/check_skill_references.py [--skills-dir skills] +""" + +from __future__ import annotations + +import argparse +import re +import sys +from pathlib import Path + +# A citation is a backtick-quoted token. Paths are recognised by their first segment matching a +# real top-level entry of the repository, which keeps prose like `@Transactional` or `--specs` +# out of the check without needing a list of exceptions. +BACKTICK = re.compile(r"`([^`\n]+)`") + +# Trailing punctuation that belongs to the sentence rather than to the path. +TRAILING_PUNCTUATION = ".,:;)]}" + + +def repository_root() -> Path: + return Path(__file__).resolve().parent.parent + + +def top_level_names(root: Path) -> set[str]: + return {entry.name for entry in root.iterdir()} + + +def candidate_paths(text: str, known_top_level: set[str]) -> list[str]: + """Extract the backtick-quoted tokens that look like repository-relative paths.""" + found = [] + for raw in BACKTICK.findall(text): + token = raw.strip().rstrip(TRAILING_PUNCTUATION) + if not token or " " in token or "/" not in token: + continue + # URLs, package names and Java FQNs are not repository paths. + if token.startswith(("http://", "https://", "//")) or token.startswith("@"): + continue + # A template such as `changelog/_changelog.xml` names a shape, not a file. + if "<" in token or ">" in token: + continue + first = token.split("/", 1)[0] + if first not in known_top_level: + continue + found.append(token) + return found + + +def path_exists(root: Path, token: str) -> bool: + """A token exists if it resolves to a file or directory, or, when it ends in a glob, matches something.""" + if token.endswith("*"): + pattern = token.rstrip("*").rstrip("/") + parent = root / pattern + if parent.is_dir(): + return any(parent.iterdir()) + # A glob like path/to/*.xml: let pathlib resolve it relative to the root. + return any(root.glob(token)) + return (root / token).exists() + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + parser.add_argument("--skills-dir", default="skills", help="Directory holding the skills (default: skills)") + args = parser.parse_args() + + root = repository_root() + skills_dir = root / args.skills_dir + + if not skills_dir.is_dir(): + print(f"ERROR: no such directory: {skills_dir}", file=sys.stderr) + return 1 + + known_top_level = top_level_names(root) + broken: list[tuple[Path, str]] = [] + checked = 0 + + for skill_file in sorted(skills_dir.rglob("*.md")): + text = skill_file.read_text(encoding="utf-8") + for token in candidate_paths(text, known_top_level): + checked += 1 + if not path_exists(root, token): + broken.append((skill_file.relative_to(root), token)) + + if broken: + print(f"{len(broken)} broken path reference(s) in {args.skills_dir}/:\n", file=sys.stderr) + for skill_file, token in broken: + print(f" {skill_file}: {token}", file=sys.stderr) + print( + "\nA skill must not cite a path that does not exist. Update the citation, or remove it " + "if the thing it described is gone.", + file=sys.stderr, + ) + return 1 + + print(f"OK: {checked} path reference(s) in {args.skills_dir}/ all resolve.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From 9b9cb909aa7d5ca63fbf43125dcd41bb2c939d6f Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 4 Sep 2026 22:58:08 +0200 Subject: [PATCH 2/9] Development: Address review of the agent skills Correctness of the skills themselves, first. A skill that states something false is worse than no skill, and review found three that did: - local-setup claimed create_test_users.sh seeds the Playwright users and that the fast E2E runner calls it. Neither is true: those users come from the Liquibase E2E changelog, and that script creates three unrelated ones and needs a server argument it was not shown with. - ci-triage's workflow/job table was wrong in four of five rows, which is the map the skill navigates by. - e2e-pr-check said the suite is ~316 tests (it is 418), that a bad --specs path yields a silent "no tests found" (Playwright errors and exits 1), and described only the harmless direction of the uncommitted-changes trap. The citation check never ran on the PRs it guards. ci-quality.yml is gated on build_relevant, which is false when every changed file is markdown, so a skills-only PR skipped it entirely. Moved to its own ci-skills.yml behind a has_skills area filter, following the has_beans precedent, and added to the required gate. The checker itself was narrower than its docstring claimed: - a non-trailing glob such as config/application-*.yml was reported broken - intra-skill reference/ links were invisible, the likeliest breakage of all - fenced code blocks were never scanned, so example commands went unchecked - a citation under a renamed top-level directory was skipped, not reported - a token with .. could escape the repository - the known-top-level set came from iterdir(), so it depended on whether the working tree happened to hold build output; it now comes from git ls-files It checks 92 distinct citations, up from 73 counted with duplicates. Also: registered the docs page in sidebar-developer.ts, which is an explicit sidebar rather than an autogenerated one, so the page was unreachable from the navigation; fixed the multinode --help off-by-one; marked the abridged SQL snippet so it is not copied as a template; documented --specs and corrected a stale ESLint rule name in CLAUDE.md; gitignored __pycache__. --- .ci/E2E-tests/determine-relevant-tests.sh | 1 + .github/workflows/ci-quality.yml | 20 -- .github/workflows/ci-skills.yml | 37 +++ .github/workflows/ci-workflows.yml | 5 + .github/workflows/ci.yml | 24 +- .gitignore | 2 + CLAUDE.md | 8 +- documentation/docs/developer/work-with-ai.mdx | 11 +- documentation/sidebar-developer.ts | 1 + run-e2e-tests-local-multinode-fast.sh | 2 +- skills/README.md | 5 +- skills/ci-triage/SKILL.md | 24 +- skills/e2e-pr-check/SKILL.md | 18 +- .../reference/migration-patterns.md | 8 + skills/local-setup/SKILL.md | 32 ++- skills/server-arch-gates/SKILL.md | 4 +- skills/write-tests/reference/client.md | 6 +- supporting_scripts/check_skill_references.py | 215 +++++++++++++----- 18 files changed, 313 insertions(+), 110 deletions(-) create mode 100644 .github/workflows/ci-skills.yml diff --git a/.ci/E2E-tests/determine-relevant-tests.sh b/.ci/E2E-tests/determine-relevant-tests.sh index 4edc8cb134b0..eb45f8e308fb 100755 --- a/.ci/E2E-tests/determine-relevant-tests.sh +++ b/.ci/E2E-tests/determine-relevant-tests.sh @@ -15,6 +15,7 @@ set -e # e2e-pr-check agent skill and anyone debugging test selection needs. if [ -z "${DETERMINE_RELEVANT_TESTS_REEXEC:-}" ] && [ "${BASH_VERSINFO[0]}" -lt 4 ]; then for candidate in "$(command -v bash || true)" /opt/homebrew/bin/bash /usr/local/bin/bash; do + # shellcheck disable=SC2016 # single quotes are required: the expansion must happen in the candidate shell if [ -x "$candidate" ] && [ "$("$candidate" -c 'echo ${BASH_VERSINFO[0]}')" -ge 4 ]; then DETERMINE_RELEVANT_TESTS_REEXEC=1 exec "$candidate" "${BASH_SOURCE[0]}" "$@" fi diff --git a/.github/workflows/ci-quality.yml b/.github/workflows/ci-quality.yml index bfa8ace93bb0..dd4763b356d2 100644 --- a/.github/workflows/ci-quality.yml +++ b/.github/workflows/ci-quality.yml @@ -267,23 +267,3 @@ jobs: echo "::error::Query Quality failed. See https://docs.artemis.tum.de/developer/guidelines/performance and https://docs.artemis.tum.de/developer/guidelines/database." exit 1 fi - - agent-skills: - name: Agent Skills - runs-on: ubuntu-latest - timeout-minutes: 5 - permissions: - contents: read - steps: - - name: Checkout code - uses: actions/checkout@v6 - with: - ref: ${{ inputs.commit_sha }} - - name: Set up Python - uses: actions/setup-python@v6 - with: - python-version: '3.14' - # A skill that cites a file which has since moved is worse than no skill: an agent acts on it - # without checking. This is a pure source scan, so it is cheap and always runs. - - name: Skill path references - run: python supporting_scripts/check_skill_references.py diff --git a/.github/workflows/ci-skills.yml b/.github/workflows/ci-skills.yml new file mode 100644 index 000000000000..ced15b3aa7d2 --- /dev/null +++ b/.github/workflows/ci-skills.yml @@ -0,0 +1,37 @@ +name: Agent Skills + +# Validates the agent skills in skills/. Kept out of ci-quality.yml on purpose: that workflow is +# gated on `build_relevant`, which is false when every changed file is markdown, so a PR editing +# only skills/ would skip the one check that guards it. + +on: + workflow_call: + inputs: + commit_sha: + description: 'SHA to check. PR runs use the PR head SHA (matching ci-build.yml).' + required: true + type: string + +permissions: {} # default-deny; the job grants only what it needs + +jobs: + skill-references: + name: Skill Path References + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + steps: + - name: Checkout code + uses: actions/checkout@v6 + with: + # A pure source scan (no git diff), so pin the PR head the way the style jobs do. + ref: ${{ inputs.commit_sha }} + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: '3.14' + # A skill that cites a file which has since moved is worse than no skill: an agent acts on it + # without checking. This is what keeps the citations honest as the code around them changes. + - name: Skill path references + run: python supporting_scripts/check_skill_references.py diff --git a/.github/workflows/ci-workflows.yml b/.github/workflows/ci-workflows.yml index 540e87ff8b70..2075a043c654 100644 --- a/.github/workflows/ci-workflows.yml +++ b/.github/workflows/ci-workflows.yml @@ -111,6 +111,11 @@ jobs: "has_gradle": {".github/workflows/ci.yml", ".github/workflows/ci-gradle-wrapper.yml"}, "has_version": {".github/workflows/ci.yml", ".github/workflows/ci-version-consistency.yml", "supporting_scripts/update_version.sh"}, "has_beans": {".github/workflows/ci.yml", ".github/workflows/ci-bean-instantiations.yml"}, + "has_skills": { + ".github/workflows/ci.yml", + ".github/workflows/ci-skills.yml", + "supporting_scripts/check_skill_references.py", + }, } failed = False diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97c7c0864627..70dabd6da426 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,6 +49,7 @@ jobs: has_workflows: ${{ steps.decide.outputs.has_workflows }} has_version: ${{ steps.decide.outputs.has_version }} has_beans: ${{ steps.decide.outputs.has_beans }} + has_skills: ${{ steps.decide.outputs.has_skills }} has_tum_ui: ${{ steps.decide.outputs.has_tum_ui }} steps: - uses: actions/checkout@v6 @@ -100,6 +101,11 @@ jobs: - 'src/main/resources/**' - 'build.gradle' - 'gradle/**' + has_skills: + - '.github/workflows/ci.yml' + - '.github/workflows/ci-skills.yml' + - 'skills/**' + - 'supporting_scripts/check_skill_references.py' has_docs: - '.github/workflows/ci.yml' - '.github/workflows/ci-docs.yml' @@ -166,11 +172,12 @@ jobs: F_WORKFLOWS: ${{ steps.area.outputs.has_workflows }} F_VERSION: ${{ steps.area.outputs.has_version }} F_BEANS: ${{ steps.area.outputs.has_beans }} + F_SKILLS: ${{ steps.area.outputs.has_skills }} F_TUM_UI: ${{ steps.area.outputs.has_tum_ui }} run: | set -Eeuo pipefail if [ "$FORCE" = "true" ]; then - for k in build_relevant has_java has_docs has_i18n has_gradle has_workflows has_version has_beans has_tum_ui; do + for k in build_relevant has_java has_docs has_i18n has_gradle has_workflows has_version has_beans has_skills has_tum_ui; do echo "$k=true" >> "$GITHUB_OUTPUT" done else @@ -183,6 +190,7 @@ jobs: echo "has_workflows=${F_WORKFLOWS:-false}" echo "has_version=${F_VERSION:-false}" echo "has_beans=${F_BEANS:-false}" + echo "has_skills=${F_SKILLS:-false}" echo "has_tum_ui=${F_TUM_UI:-false}" } >> "$GITHUB_OUTPUT" fi @@ -568,6 +576,18 @@ jobs: with: commit_sha: ${{ github.event.pull_request.head.sha || github.sha }} + # Not folded into `quality`: that job is gated on `build_relevant`, which is false when every + # changed file is markdown. A PR editing only skills/ would then skip the check that guards it. + skills: + name: Agent Skills + needs: detect-changes + if: needs.detect-changes.outputs.has_skills == 'true' + uses: ./.github/workflows/ci-skills.yml + permissions: + contents: read + with: + commit_sha: ${{ github.event.pull_request.head.sha || github.sha }} + # Required for internal PRs — part of the `all-required-ci-passed` gate (a test that fails all # its retries blocks the merge; flaky-recovered runs stay green). Depends on `build` for the # Docker image, not on `test`, so a flaky unit-test run never blocks it; runs only when `build` @@ -651,6 +671,7 @@ jobs: - workflows - version-consistency - bean-instantiations + - skills - e2e runs-on: ubuntu-latest permissions: {} # reads only the `needs` context; no checkout, no token scope @@ -717,6 +738,7 @@ jobs: - translation - version-consistency - bean-instantiations + - skills - e2e - codeql - coverage-report diff --git a/.gitignore b/.gitignore index 73cee7ff260f..0ec6ed95ec2c 100644 --- a/.gitignore +++ b/.gitignore @@ -262,6 +262,8 @@ deferredEagerBeanInstantiationViolations.dot ###################### /src/test/playwright/monocart-report /venv/ +# Running any supporting_scripts/*.py leaves one of these behind +__pycache__/ /src/test/playwright/test-exercise-repos /docker/playwright-multinode-arch-override.yml /docker/playwright-multinode-middleware-override.yml diff --git a/CLAUDE.md b/CLAUDE.md index 633b781440e9..2b3a0e9e6f9e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,8 +90,13 @@ pnpm run vitest -- path/to/spec.ts # Single Vitest file ./run-e2e-tests-local-fast.sh # Run all E2E tests ./run-e2e-tests-local-fast.sh --filter "Quiz" # Run tests matching "Quiz" ./run-e2e-tests-local-fast.sh --filter "ExamAssessment|SystemHealth" # Multiple patterns +./run-e2e-tests-local-fast.sh --specs "e2e/exam/ExamResults.spec.ts e2e/lecture/" # Only these spec paths ./run-e2e-tests-local-fast.sh --stop # Stop all services +# --filter is Playwright --grep (matches test TITLES); --specs replaces the spec PATHS that run. +# For "only what my branch changed", resolve the paths first with the same script CI uses: +./.ci/E2E-tests/determine-relevant-tests.sh origin/develop # prints RELEVANT_TESTS=... + # Multi-node E2E (catches cluster / cache coherence regressions) # Boots the full production-faithful stack: Postgres, JHipster Registry (Eureka), # ActiveMQ, 3 Artemis nodes, nginx LB, containerised Playwright. Slower than the @@ -108,6 +113,7 @@ pnpm run vitest -- path/to/spec.ts # Single Vitest file # containers. Use this for server-side iteration on multi-node bugs. Cold ~1–2 min, warm ~30 s. ./run-e2e-tests-local-multinode-fast.sh # Full run (build WAR + infra + 3 host JVMs + tests) ./run-e2e-tests-local-multinode-fast.sh --filter "Quiz" # Filter to a subset of tests +./run-e2e-tests-local-multinode-fast.sh --specs "e2e/exam/" # Only these spec paths ./run-e2e-tests-local-multinode-fast.sh --middleware redis # Same suite, Redis instead of Hazelcast ./run-e2e-tests-local-multinode-fast.sh --skip-build --skip-up # Re-run tests against the running stack ./run-e2e-tests-local-multinode-fast.sh --stop # Tear everything down @@ -227,7 +233,7 @@ Organized by feature module: - Use `inject()` for dependency injection instead of constructor injection - Legacy decorators (`@Input`, `@Output`, `@ViewChild`, `@ViewChildren`, `@ContentChild`, `@ContentChildren`) must not be used in new code - In modules not yet fully migrated, prefer signal-based APIs for new components but maintain consistency within existing components - - An ESLint rule (`enforce-signal-apis-in-migrated-modules`) enforces this in fully migrated modules + - An ESLint rule (`localRules/enforce-signal-apis`, in `rules/enforce-signal-apis.mjs`) enforces this in fully migrated modules - **`ngOnChanges` is banned — use `computed()`/`effect()` instead.** An error-level rule (`localRules/prefer-signal-reactivity-over-ngonchanges`) enforces this across `src/main/webapp/app`, `packages/tum-ui/src/lib`, and `src/test/javascript`, including specs and undecorated base classes. Angular 21 does call inherited `ngOnChanges` hooks and fires them for signal inputs, so this is a consistency ban rather than a correctness fix. A genuinely unavoidable use of `SimpleChanges.previousValue`/`isFirstChange()` or pre-child-initialization ordering needs a detailed comment and a justified line-level `eslint-disable-next-line`. `ngOnInit` and `ngOnDestroy` are unaffected. See `documentation/docs/developer/guidelines/client-development.mdx`. - **Angular template control flow: use `@if`, `@for`, `@switch`; never use `*ngIf`, `*ngFor`, `*ngSwitch`** - Avoid `null`, use `undefined` where possible diff --git a/documentation/docs/developer/work-with-ai.mdx b/documentation/docs/developer/work-with-ai.mdx index 94a457d160f1..9da8dd5a9e5e 100644 --- a/documentation/docs/developer/work-with-ai.mdx +++ b/documentation/docs/developer/work-with-ai.mdx @@ -47,7 +47,7 @@ Code, Cursor, Codex, Copilot, Cline, opencode, Zed, Windsurf, Gemini CLI, and ma Claude Code can install them as a versioned plugin, which namespaces the skills and lets you update them with a single command: -``` +```text /plugin marketplace add ls1intum/Artemis /plugin install artemis@artemis ``` @@ -76,7 +76,7 @@ immediately. | Skill | Use it when | | --------------------- | ---------------------------------------------------------------------------- | -| `e2e-pr-check` | You want the E2E tests a change affects, not all 316 of them | +| `e2e-pr-check` | You want the E2E tests a change affects, not all 400-plus of them | | `ci-triage` | A pull request is red, or a check never appeared | | `server-arch-gates` | You changed Java under `src/main/java` and want the gates to pass first time | | `liquibase-migration` | You are adding, changing, or dropping anything in the database schema | @@ -86,7 +86,7 @@ immediately. ### Example prompts -``` +```text Run the E2E tests my branch affects Why is PR 13652 red? Add a NOT NULL constraint on complaint.result_id @@ -129,7 +129,7 @@ rather than spread or `structuredClone`, and TUM UI with semantic colour tokens. A skill is a directory under `skills/` with a `SKILL.md` and optional `reference/` files: -``` +```text skills/ my-skill/ SKILL.md @@ -154,7 +154,8 @@ Four rules for this repository: 1. **Every factual claim cites a repository path.** [`supporting_scripts/check_skill_references.py`](https://github.com/ls1intum/Artemis/blob/develop/supporting_scripts/check_skill_references.py) - runs in the Quality workflow and fails if a cited path no longer exists. + runs as the `Agent Skills` CI job and fails if a cited path no longer exists. It reads both + inline code spans and fenced code blocks, so example commands are checked too. 2. **Keep the body a procedure.** Background goes in a `reference/` file, which costs nothing until the agent reads it. 3. **Say why, not just what.** A rule without its reason gets worked around instead of followed. diff --git a/documentation/sidebar-developer.ts b/documentation/sidebar-developer.ts index ff8d9e3bebf7..fd855ce2bf5d 100644 --- a/documentation/sidebar-developer.ts +++ b/documentation/sidebar-developer.ts @@ -5,6 +5,7 @@ const sidebars: SidebarsConfig = { 'intro', 'setup', 'development-process', + 'work-with-ai', 'reviewer-guidelines', { type: 'category', diff --git a/run-e2e-tests-local-multinode-fast.sh b/run-e2e-tests-local-multinode-fast.sh index 4a0e323ec8b4..e6c87da3cc23 100755 --- a/run-e2e-tests-local-multinode-fast.sh +++ b/run-e2e-tests-local-multinode-fast.sh @@ -102,7 +102,7 @@ while [[ $# -gt 0 ]]; do TEST_SPECS="$2" shift 2 ;; - --help) head -46 "$0" | tail -42; exit 0 ;; + --help) head -45 "$0" | tail -41; exit 0 ;; *) echo -e "${RED}Unknown option: $1${NC}"; exit 1 ;; esac done diff --git a/skills/README.md b/skills/README.md index c2c2e3238e52..1bed46b60f88 100644 --- a/skills/README.md +++ b/skills/README.md @@ -16,7 +16,7 @@ npx skills add ls1intum/Artemis Claude Code, as a versioned plugin with namespaced skills (`/artemis:e2e-pr-check`): -``` +```text /plugin marketplace add ls1intum/Artemis /plugin install artemis@artemis ``` @@ -52,7 +52,8 @@ The procedure. Rules for this repository: - **Every factual claim cites a repository path.** `supporting_scripts/check_skill_references.py` - runs in CI and fails if a path referenced from `skills/` no longer exists. + runs as the `Agent Skills` CI job and fails if a path referenced from `skills/` no longer exists. + It reads inline code spans and fenced code blocks, so example commands are checked too. - **Keep the body a procedure.** Long background belongs in a `reference/` file, which costs nothing until the agent reads it. - **Say why, not just what.** A rule without its reason gets worked around rather than followed. diff --git a/skills/ci-triage/SKILL.md b/skills/ci-triage/SKILL.md index 9bc38c9bafae..e02f8044d725 100644 --- a/skills/ci-triage/SKILL.md +++ b/skills/ci-triage/SKILL.md @@ -21,15 +21,21 @@ The only check that gates a merge is the aggregate **All required CI Passed** establish whether the failing job is inside that gate before treating it as urgent. Report PR Coverage is deliberately outside it. -The workflows and the jobs they contain: - -| Workflow | Jobs | -| ---------------------------------------------- | -------------------------------------------------------------------------------------------------- | -| `.github/workflows/ci-quality.yml` | Server Code Style, Client Code Style, Client Compilation, Server Code Quality, Query Quality Check | -| `.github/workflows/ci-test.yml` | Server Tests (PostgreSQL), Client Tests | -| `.github/workflows/ci-e2e.yml` | Determine Relevant Tests, Phase 1 and Phase 2 E2E, Report E2E Overall Status | -| `.github/workflows/ci-build.yml` | Build | -| `.github/workflows/ci-bean-instantiations.yml` | Bean instantiation count gate | +The workflows and the jobs they contain. `ci.yml` calls each one under a shorter caller name (for +example `Build`, `Quality`, `Test`), so a check named `Quality / Server Code Style` is the +`server-style` job of `ci-quality.yml`: + +| Workflow | Jobs | +| ---------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `.github/workflows/ci-quality.yml` | Server Code Style, Client Code Style, Client Compilation, Server Code Quality, Query Quality Check | +| `.github/workflows/ci-test.yml` | Server Tests (PostgreSQL), Client Tests | +| `.github/workflows/ci-e2e.yml` | Determine Relevant Tests; Phase 1: Relevant E2E Tests; Phase 2: Remaining E2E Tests; Run All E2E Tests (PR); Run All E2E Tests (Non-PR); Report E2E Overall Status | +| `.github/workflows/ci-build.yml` | Build .war artifact, Upload Release Artifact, Build and Push Docker Image (PR, amd64), Build and Push Docker Image, Save Docker Image Tag, Sign and Attest Docker Image | +| `.github/workflows/ci-bean-instantiations.yml` | Bean Instantiation Check | +| `.github/workflows/ci-skills.yml` | Skill Path References | + +Note that `Run All E2E Tests (Non-PR)` is the job that runs on develop. A spec can fail there and +pass in a pull request's phased run, because the two do not use the same topology. ## Step 2: match against the known patterns diff --git a/skills/e2e-pr-check/SKILL.md b/skills/e2e-pr-check/SKILL.md index 4df0b301df07..44679423d992 100644 --- a/skills/e2e-pr-check/SKILL.md +++ b/skills/e2e-pr-check/SKILL.md @@ -5,8 +5,8 @@ description: Run the Artemis Playwright E2E tests that this branch's changes act # Run the E2E tests this change affects -The full Playwright suite is roughly 316 tests and takes tens of minutes. Almost no change needs -all of them. This skill selects the specs the change actually affects, runs them, and then reads +The full Playwright suite is over 400 tests across roughly 90 spec files, and takes tens of +minutes. Almost no change needs all of them. This skill selects the specs the change actually affects, runs them, and then reads the result with the failure patterns of this suite in mind. ## Step 1: work out which specs are affected @@ -33,8 +33,11 @@ It prints five `OUTPUT:` lines. The ones that matter: Two things about the input: - **It diffs commits, not the working tree.** The script runs `git diff --name-only ...HEAD`, - so uncommitted changes are invisible to it and it reports "No changed files detected. Running all - tests." Commit first, or the selection will be wrong in the direction of running everything. + so uncommitted changes are invisible to it. **Commit before resolving.** With nothing committed at + all it says "No changed files detected. Running all tests.", which is loud and harmless. The + dangerous case is quieter: committed work plus uncommitted edits touching a further module gives + a selection based only on the committed files, so the specs covering your newest edits are the + ones left out. - **Pass a different base for a stacked branch.** The base is the first argument. A stacked pull request is not cut from develop, so diffing against develop selects its parent's changes too. @@ -104,9 +107,10 @@ locally and fails in CI with an off-by-a-few count is usually this. and the test stays flaky in CI where the machine is slower and more loaded. Find what the test is actually waiting for. -**Did the run produce zero tests?** `--specs` paths are relative to `src/test/playwright`, and a -typo yields "no tests found" rather than an error. Check the count in the summary matches what you -expected from step 1. +**Is the failure just a bad `--specs` path?** Paths are relative to `src/test/playwright`. A typo +makes Playwright print `Error: No tests found.` and exit non-zero, which the runner reports as a +failed run. So a red run with no test output at all is a path problem, not a test problem. Check +the executed count against what step 1 selected before reading anything else. ## Reporting back diff --git a/skills/liquibase-migration/reference/migration-patterns.md b/skills/liquibase-migration/reference/migration-patterns.md index 4f7b70431735..e243e2bcffd4 100644 --- a/skills/liquibase-migration/reference/migration-patterns.md +++ b/skills/liquibase-migration/reference/migration-patterns.md @@ -12,10 +12,18 @@ that no such row exists. The pattern is two changesets per column: one that clears the rows without a parent, and one that adds the constraint behind a precondition. +The snippet below is **abridged to show the shape**. Do not copy it as a template: the real +changeset also clears `long_feedback_text`, `text_block`, `result_rating`, `assessment_note`, +`complaint_response` and `complaint`, and nulls three foreign key columns, in that order. Deleting +a parent before its dependants dies on a foreign key constraint, which is exactly the +application-will-not-start failure this skill opens with. Work out the full dependency order for +your own table, and read the complete changeset cited below. + ```xml Remove results that belong to no submission, along with everything that hangs off them. + DELETE FROM feedback WHERE result_id IN (SELECT id FROM result WHERE submission_id IS NULL); DELETE FROM result WHERE submission_id IS NULL; diff --git a/skills/local-setup/SKILL.md b/skills/local-setup/SKILL.md index 404bbd971c65..331248f55e30 100644 --- a/skills/local-setup/SKILL.md +++ b/skills/local-setup/SKILL.md @@ -17,9 +17,14 @@ description: Get a local Artemis development environment running from a fresh cl Run `corepack enable` once. It activates the exact pnpm version the repository pins, which avoids a whole category of lockfile arguments. -On macOS, Homebrew's `openjdk@25` is keg-only, so it is not on the path after installation. Symlink -it rather than exporting `JAVA_HOME` in each shell; a permanent symlink means Gradle finds it -without a per-command prefix. +On macOS, Homebrew's `openjdk@25` is keg-only, so nothing finds it after installation. Register it +with the system once, rather than exporting `JAVA_HOME` in every shell: + +```bash +brew install openjdk@25 +sudo ln -sfn "$(brew --prefix openjdk@25)/libexec/openjdk.jdk" /Library/Java/JavaVirtualMachines/openjdk-25.jdk +./gradlew --version # confirms Gradle picks up JVM 25 +``` ## Install dependencies @@ -56,14 +61,23 @@ initialisation already defers. ## Test users -The E2E tooling creates the Playwright test users: +The users you log in as locally are seeded by Liquibase, not created by a script. The E2E changelog +`src/main/resources/config/liquibase/e2e/users.csv` provides `artemis_admin` and +`artemis_test_user_1` through `artemis_test_user_20`, each with its login as the password. The +Playwright suite reads them from `src/test/playwright/support/users.ts`. So a database that has run +the migrations already has them, and `src/test/playwright/init/importUsers.spec.ts` verifies that +rather than creating anything. + +`supporting_scripts/create_test_users.sh` is a different, much smaller thing: it creates three +users, `aa01aaa` through `aa03aaa`, through the admin REST API, and it takes the server as a +required argument: ```bash -supporting_scripts/create_test_users.sh +supporting_scripts/create_test_users.sh localhost:8080 ``` -The fast E2E runner does this as part of its setup, so if you have run -`./run-e2e-tests-local-fast.sh` you already have them. +Called without that argument it POSTs to `http://` and silently does nothing. You do not need it +for normal development or for Playwright. ## Seeing outgoing mail @@ -84,8 +98,8 @@ train. Both are pinned in `gradle.properties`. readiness and liveness endpoints and look for "Started ArtemisApp" in the log; a single unconfigured optional integration pulls the aggregate down. -**Port already in use.** The E2E runner kills processes on 8080, 9000, and 7921 before starting. -`./run-e2e-tests-local-fast.sh --stop` is a quick way to clear all three. +**Port already in use.** `./run-e2e-tests-local-fast.sh --stop` frees 8080 and 9000 by killing the +server and client. The LocalVC SSH listener on 7921 lives inside the server JVM, so it goes with it. ## Running things diff --git a/skills/server-arch-gates/SKILL.md b/skills/server-arch-gates/SKILL.md index 118bf9d78d98..f1ae199dfc37 100644 --- a/skills/server-arch-gates/SKILL.md +++ b/skills/server-arch-gates/SKILL.md @@ -5,8 +5,8 @@ description: Check Artemis server code against the architectural rules the build # Server architecture gates -Artemis enforces its server conventions with roughly 174 ArchUnit test classes under -`src/test/java`. They are not style preferences. Each one exists because the pattern it forbids +Artemis enforces its server conventions with a large ArchUnit suite under `src/test/java`, most of +it module-scoped subclasses of a handful of abstract rule bases. They are not style preferences. Each one exists because the pattern it forbids produced a production bug. The failure messages are often terse, so this skill maps a change to the rules it is subject to and to the reason behind each. diff --git a/skills/write-tests/reference/client.md b/skills/write-tests/reference/client.md index d906bf27d66f..2875b6b016a1 100644 --- a/skills/write-tests/reference/client.md +++ b/skills/write-tests/reference/client.md @@ -31,8 +31,10 @@ Run `compile:tests` before pushing any spec change. ## Signal inputs in specs A component using `input()` is driven in a spec through the component ref, not by assigning a -field. A `model()` is a two-way binding: replacing it with an `input()` plus an `output()` makes the -parent stop receiving updates, and the spec will not necessarily catch that. +field. A `model()` is a writable signal plus the matching change output that `[(name)]` binds to. +An `input()` plus an `output()` can preserve that binding, but only if the output is named +`Change` and is actually emitted on every write. Miss either and the parent silently stops +receiving updates, which a spec driving the child directly will not catch. Prefer `model()`. `MockProvider` does not stub a signal that a service initialises as a field. If a component reads a shared signal from a service, provide the real service or an explicit stub object; a `MockProvider` diff --git a/supporting_scripts/check_skill_references.py b/supporting_scripts/check_skill_references.py index a991d6333c9b..aaf7b08173fb 100644 --- a/supporting_scripts/check_skill_references.py +++ b/supporting_scripts/check_skill_references.py @@ -6,10 +6,27 @@ because it is confidently wrong and an agent will act on it. This check is what keeps that from happening silently. -It scans every file under skills/ for backtick-quoted tokens that look like repository-relative -paths, resolves each against the repository root, and fails listing the ones that do not exist. -Code blocks are included on purpose: an example command naming a stale path is exactly the case -that does the most damage. +It reads every Markdown file under skills/ and collects path-shaped tokens from two places: inline +code spans (single backticks) and the contents of fenced code blocks, where the example commands +live. A stale path in an example command is the one that does the most damage, so both are scanned. + +Four kinds of citation are resolved: + + * repository-relative, recognised by the first segment being a tracked top-level entry + (`src/main/java/...`, `.github/workflows/ci.yml`); + * skill-relative, resolved against the directory of the citing file (`reference/gates.md`), + which is the citation most likely to break and the one a repo-root check cannot see; + * repo-root scripts written with a leading `./` (`./run-e2e-tests-local-fast.sh`); + * anything ending in a known source extension, tried against both the repository root and + src/test/playwright (Playwright spec paths are written relative to the latter). This is what + still reports a citation whose top-level directory has been renamed away. + +Tokens containing a glob character must match at least one file rather than exist literally. Tokens +containing `<` or `>` are templates naming a shape, not a file, and are skipped. A token resolving +outside the repository, via `..`, counts as missing rather than as present. + +The set of known top-level entries comes from `git ls-files`, not from a directory listing, so the +result does not depend on whether the working tree happens to hold build output. Usage: python3 supporting_scripts/check_skill_references.py [--skills-dir skills] @@ -19,84 +36,180 @@ import argparse import re +import subprocess import sys from pathlib import Path -# A citation is a backtick-quoted token. Paths are recognised by their first segment matching a -# real top-level entry of the repository, which keeps prose like `@Transactional` or `--specs` -# out of the check without needing a list of exceptions. +# A citation is an inline code span. Path-shaped ones are recognised by their first segment, which +# keeps prose such as `@Transactional` or `--specs` out of the check without a list of exceptions. BACKTICK = re.compile(r"`([^`\n]+)`") +# Fenced blocks carry the example commands. Their content has no backticks, so they need +# their own pass. +FENCE = re.compile(r"^\s*```") + # Trailing punctuation that belongs to the sentence rather than to the path. TRAILING_PUNCTUATION = ".,:;)]}" +# Shell and Markdown noise wrapped around a path inside a fenced block. +SURROUNDING_NOISE = "\"'`(),;:" + +GLOB_CHARACTERS = "*?[" + +# Extensions that make a token a file citation even when its first segment is not a tracked +# top-level entry, which is how a reference to a renamed or deleted directory still gets reported. +FILE_SUFFIXES = ( + ".java", + ".ts", + ".mjs", + ".js", + ".py", + ".sh", + ".md", + ".mdx", + ".xml", + ".yml", + ".yaml", + ".json", + ".csv", + ".html", + ".scss", +) + def repository_root() -> Path: + """The repository root, two levels up from this script.""" return Path(__file__).resolve().parent.parent -def top_level_names(root: Path) -> set[str]: - return {entry.name for entry in root.iterdir()} - - -def candidate_paths(text: str, known_top_level: set[str]) -> list[str]: - """Extract the backtick-quoted tokens that look like repository-relative paths.""" - found = [] - for raw in BACKTICK.findall(text): - token = raw.strip().rstrip(TRAILING_PUNCTUATION) - if not token or " " in token or "/" not in token: - continue - # URLs, package names and Java FQNs are not repository paths. - if token.startswith(("http://", "https://", "//")) or token.startswith("@"): - continue - # A template such as `changelog/_changelog.xml` names a shape, not a file. - if "<" in token or ">" in token: +def tracked_top_level_names(root: Path) -> set[str]: + """Top-level entries git tracks, used to recognise a repository-relative token. + + Derived from the index rather than from `iterdir()` so that an untracked `build/` or + `node_modules/` in a developer's working tree cannot change the outcome relative to CI. + """ + result = subprocess.run( + ["git", "-C", str(root), "ls-files", "-z"], + capture_output=True, + check=True, + ) + entries = result.stdout.decode("utf-8").split("\0") + return {entry.split("/", 1)[0] for entry in entries if entry} + + +def code_block_tokens(text: str) -> list[str]: + """Whitespace-separated tokens from inside fenced code blocks, stripped of shell noise.""" + tokens: list[str] = [] + inside = False + for line in text.splitlines(): + if FENCE.match(line): + inside = not inside continue - first = token.split("/", 1)[0] - if first not in known_top_level: + if not inside: continue - found.append(token) - return found - - -def path_exists(root: Path, token: str) -> bool: - """A token exists if it resolves to a file or directory, or, when it ends in a glob, matches something.""" - if token.endswith("*"): - pattern = token.rstrip("*").rstrip("/") - parent = root / pattern - if parent.is_dir(): - return any(parent.iterdir()) - # A glob like path/to/*.xml: let pathlib resolve it relative to the root. - return any(root.glob(token)) - return (root / token).exists() + for word in line.split(): + tokens.append(word.strip(SURROUNDING_NOISE).rstrip(TRAILING_PUNCTUATION)) + return tokens + + +def is_path_shaped(token: str) -> bool: + """Whether the token could be a path at all, before deciding what it is relative to.""" + if not token or " " in token or "/" not in token: + return False + # URLs, package names, Java FQNs, and slash commands such as `/artemis:e2e-pr-check` are not + # repository paths. Anything starting with "/" is absolute, so it is never repository-relative. + if token.startswith(("http://", "https://", "/", "@")): + return False + # A template such as `changelog/_changelog.xml` names a shape, not a file. + return "<" not in token and ">" not in token + + +def bases_for(token: str, root: Path, skill_dir: Path, known_top_level: set[str]) -> list[Path]: + """The bases a token may be relative to, or an empty list when it is not a citation to check. + + A token resolving under any one of them counts as present. `e2e/...` paths are the reason there + is more than one: the runners take them relative to the Playwright directory, not to the root. + """ + if token.startswith("./"): + return [root] + first = token.split("/", 1)[0] + if first in known_top_level: + return [root] + # A skill's own reference files are cited relative to the skill directory. + if (skill_dir / first).exists(): + return [skill_dir] + # Names a file, but under a first segment git does not track: either a Playwright-relative spec + # path, or a genuinely stale citation to a directory that has been renamed or removed. + if token.endswith(FILE_SUFFIXES): + return [root, root / "src" / "test" / "playwright"] + return [] + + +def path_exists(base: Path, token: str, root: Path) -> bool: + """Whether the token resolves under base, matching at least one file when it is a glob. + + A token containing `..` could otherwise escape the repository and report a file outside it as + present, so anything resolving outside `root` counts as missing. + """ + relative = token[2:] if token.startswith("./") else token + if any(character in relative for character in GLOB_CHARACTERS): + try: + matches = list(base.glob(relative)) + except (ValueError, NotImplementedError): + # An unsupported pattern (for example a bare trailing '**') is not a broken citation. + return True + return any(root in match.resolve().parents for match in matches) + candidate = (base / relative).resolve() + if root != candidate and root not in candidate.parents: + return False + return candidate.exists() def main() -> int: - parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) - parser.add_argument("--skills-dir", default="skills", help="Directory holding the skills (default: skills)") + """Scan the skills directory and report every cited repository path that no longer exists.""" + parser = argparse.ArgumentParser( + description=__doc__, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + parser.add_argument( + "--skills-dir", + default="skills", + help="Directory holding the skills (default: skills)", + ) args = parser.parse_args() root = repository_root() - skills_dir = root / args.skills_dir + skills_dir = (root / args.skills_dir).resolve() if not skills_dir.is_dir(): print(f"ERROR: no such directory: {skills_dir}", file=sys.stderr) return 1 + if root not in skills_dir.parents and skills_dir != root: + print(f"ERROR: --skills-dir must be inside the repository: {skills_dir}", file=sys.stderr) + return 1 - known_top_level = top_level_names(root) - broken: list[tuple[Path, str]] = [] - checked = 0 + known_top_level = tracked_top_level_names(root) + # Sets, because a path cited both in prose and in an example command is one citation, not two. + broken: set[tuple[Path, str]] = set() + checked: set[tuple[Path, str]] = set() for skill_file in sorted(skills_dir.rglob("*.md")): text = skill_file.read_text(encoding="utf-8") - for token in candidate_paths(text, known_top_level): - checked += 1 - if not path_exists(root, token): - broken.append((skill_file.relative_to(root), token)) + relative_file = skill_file.relative_to(root) + inline = (raw.strip().rstrip(TRAILING_PUNCTUATION) for raw in BACKTICK.findall(text)) + for token in list(inline) + code_block_tokens(text): + if not is_path_shaped(token): + continue + bases = bases_for(token, root, skill_file.parent, known_top_level) + if not bases: + continue + checked.add((relative_file, token)) + if not any(path_exists(base, token, root) for base in bases): + broken.add((relative_file, token)) if broken: print(f"{len(broken)} broken path reference(s) in {args.skills_dir}/:\n", file=sys.stderr) - for skill_file, token in broken: + for skill_file, token in sorted(broken): print(f" {skill_file}: {token}", file=sys.stderr) print( "\nA skill must not cite a path that does not exist. Update the citation, or remove it " @@ -105,7 +218,7 @@ def main() -> int: ) return 1 - print(f"OK: {checked} path reference(s) in {args.skills_dir}/ all resolve.") + print(f"OK: {len(checked)} distinct path reference(s) in {args.skills_dir}/ all resolve.") return 0 From 187d631a3b2546622266ecc4f2397cea103579b7 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 4 Sep 2026 23:01:37 +0200 Subject: [PATCH 3/9] Development: Document why slash-free citations are not checked Copilot suggested also validating bare tokens such as CLAUDE.md. Measured against the current skills, that reports 8 of 13 as broken: *Test.java is a naming rule, ArchitectureTest.java a class, SKILL.md a kind of file, ci.yml a workflow named by its basename. The subset that would be safe, a bare name that is a tracked top-level file, can never fail, because the token is only recognised as a path because it already exists. --- supporting_scripts/check_skill_references.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/supporting_scripts/check_skill_references.py b/supporting_scripts/check_skill_references.py index aaf7b08173fb..5d4c678e9b4e 100644 --- a/supporting_scripts/check_skill_references.py +++ b/supporting_scripts/check_skill_references.py @@ -25,6 +25,13 @@ containing `<` or `>` are templates naming a shape, not a file, and are skipped. A token resolving outside the repository, via `..`, counts as missing rather than as present. +A token needs a `/` to be considered at all. Slash-free ones are prose far more often than they are +citations: `*Test.java` is a naming rule, `ArchitectureTest.java` is a class, `SKILL.md` is a kind +of file, `ci.yml` is a workflow referred to by its basename. Checking them would report all of +those as broken. The subset that could be checked safely, a bare name that is a tracked top-level +file, is tautological: such a token is only recognised because it exists, so it can never fail. +Cite a root file with a directory-bearing path if you want it validated. + The set of known top-level entries comes from `git ls-files`, not from a directory listing, so the result does not depend on whether the working tree happens to hold build output. From fe5d382fb25c752f29adf92bf5fcce9b6c4d1c20 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 4 Sep 2026 23:16:44 +0200 Subject: [PATCH 4/9] Development: Correct the seeded test users and the Vitest single-file command Two more claims that did not survive checking against the source. local-setup said the E2E changelog seeds artemis_test_user_1 through _20. It seeds seven users and the numbering is not contiguous: 1, 2, 3, 4, 6, 16, plus artemis_admin. A reader following the old text would look for artemis_test_user_5 and not find it. Replaced with the actual list and the Playwright name each maps to. CLAUDE.md documented `pnpm run vitest -- path/to/spec.ts` as the way to run a single file. Measured: the path is not forwarded as a filter and the whole suite runs, 1298 files instead of 1. This is the same trap write-tests warns about, so the facts file contradicted the skill. Corrected to `pnpm exec vitest run ` with a note about the form that does not work. Also dropped a "wastes 40 minutes" figure from ci-triage for a claim that does not go stale. --- CLAUDE.md | 6 ++++-- skills/ci-triage/SKILL.md | 2 +- skills/local-setup/SKILL.md | 24 ++++++++++++++++++------ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2b3a0e9e6f9e..4ffb21dda708 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,9 +81,11 @@ pnpm run prettier:write # Fix formatting # Client (Vitest - preferred for new tests) pnpm run vitest # Watch mode -pnpm run vitest:run # Single run +pnpm run vitest:run # Single run, whole suite pnpm run vitest:coverage # With coverage -pnpm run vitest -- path/to/spec.ts # Single Vitest file +pnpm exec vitest run path/to/spec.ts # Single Vitest file +# NOT `pnpm run vitest:run -- path/to/spec.ts`: the path is not forwarded as a filter and the +# whole suite runs (1298 files instead of 1). Use `pnpm exec vitest run ` as shown above. # E2E Tests (Playwright) — preferred way to run locally # The script auto-kills processes on ports 8080/9000/7921, starts Postgres, server, and client. diff --git a/skills/ci-triage/SKILL.md b/skills/ci-triage/SKILL.md index e02f8044d725..fe1aced2e45a 100644 --- a/skills/ci-triage/SKILL.md +++ b/skills/ci-triage/SKILL.md @@ -51,7 +51,7 @@ defect. It covers, with the tell for each: ## Step 3: re-run correctly -Re-running the wrong way wastes 40 minutes and produces a confusing result. +Re-running the wrong way wastes a full CI cycle and produces a confusing result. - **Server Tests: never use `gh run rerun --failed`.** The suite's sharding and reporting mean a partial re-run does not reproduce the original conditions. Use a full re-run: diff --git a/skills/local-setup/SKILL.md b/skills/local-setup/SKILL.md index 331248f55e30..6eed308622f3 100644 --- a/skills/local-setup/SKILL.md +++ b/skills/local-setup/SKILL.md @@ -61,12 +61,24 @@ initialisation already defers. ## Test users -The users you log in as locally are seeded by Liquibase, not created by a script. The E2E changelog -`src/main/resources/config/liquibase/e2e/users.csv` provides `artemis_admin` and -`artemis_test_user_1` through `artemis_test_user_20`, each with its login as the password. The -Playwright suite reads them from `src/test/playwright/support/users.ts`. So a database that has run -the migrations already has them, and `src/test/playwright/init/importUsers.spec.ts` verifies that -rather than creating anything. +The users you log in as locally are seeded by Liquibase, not created by a script. +`src/main/resources/config/liquibase/e2e/users.csv` provides exactly seven, each with its login as +the password: + +| Login | Role in the Playwright suite | +| ---------------------- | ---------------------------- | +| `artemis_admin` | `admin` | +| `artemis_test_user_1` | `studentOne` | +| `artemis_test_user_2` | `studentTwo` | +| `artemis_test_user_3` | `studentThree` | +| `artemis_test_user_4` | `studentFour` | +| `artemis_test_user_6` | `tutor` | +| `artemis_test_user_16` | `instructor` | + +The numbering is deliberately not contiguous, so do not assume `artemis_test_user_5` exists. The +names are exported from `src/test/playwright/support/users.ts`. A database that has run the +migrations already has these users, and `src/test/playwright/init/importUsers.spec.ts` verifies +them rather than creating anything. `supporting_scripts/create_test_users.sh` is a different, much smaller thing: it creates three users, `aa01aaa` through `aa03aaa`, through the admin REST API, and it takes the server as a From 19e95eb0aa56e52397a54ce9b3edba9d9d15267a Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 5 Sep 2026 00:33:27 +0200 Subject: [PATCH 5/9] Development: Follow develop's per-node cache rename, and scan tilde fences Merging develop brought in #13648, which renamed BlobCacheEvictionService to PerNodeCacheEvictionService and started serving titles per node too. The citation check caught the stale path in server-arch-gates within seconds of the merge, which is a fair demonstration of why it exists. Updated the caching guidance for what the code now does: RoutingCacheManager routes both BLOB_CACHE_NAMES and the new TITLE_CACHE_NAMES to per-node Caffeine and everything else to the distributed provider, every per-node cache expires on a TTL, and PerNodeCacheEvictionService broadcasts evictions over a plain topic because a dropped broadcast self-corrects within that TTL. CLAUDE.md carried the same stale class name and is corrected with it. Also, per review, the checker now recognises tilde fences (~~~) as well as backtick fences, so a tilde-fenced example block is no longer skipped. --- CLAUDE.md | 2 +- skills/server-arch-gates/reference/gates.md | 26 +++++++++++++++----- supporting_scripts/check_skill_references.py | 5 ++-- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 578e9d5a7c7a..6b42082abbab 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -222,7 +222,7 @@ Organized by feature module: ### Caching - **Do not add `@Cache` (Hibernate L2) annotations on entities or associations.** Hibernate second-level cache is disabled cluster-wide and an ArchUnit rule (`ArchitectureTest.testNoHibernateSecondLevelCacheAnnotation`) fails the build if any reappears. Reason: `@Modifying @Query` repository methods bypass L2 invalidation, and the absence of service-level `@Transactional` leaves no clean place to coordinate eviction within a REST call — both produced cross-node stale-read bugs in the multi-node cluster (issue #12574, fixed in PR #12578; further cleanup in PR #12579). -- **For DTO / projection caching, use Spring `@Cacheable`.** It resolves against the `RoutingCacheManager` in `core/config/cache/CacheManagerConfiguration`, which serves blob caches (`files`, `plantUmlPng`, `plantUmlSvg`) from a bounded per-node Caffeine cache and every other cache from the distributed data provider. Always pair `@Cacheable` with explicit eviction — `@CacheEvict` on the writer service, or a Hibernate `PostUpdateEventListener` / `PostDeleteEventListener`. See `TitleCacheEvictionService` for the canonical pattern, and `BlobCacheEvictionService` for evicting a per-node blob cache across the cluster. +- **For DTO / projection caching, use Spring `@Cacheable`.** It resolves against the `RoutingCacheManager` in `core/config/cache/CacheManagerConfiguration`, which serves the per-node caches from a bounded Caffeine cache and every other cache from the distributed data provider. The per-node ones are the blobs of `BlobCacheConfiguration` (`files`, `plantUmlPng`, `plantUmlSvg`) and the titles of `TitleCacheConfiguration`; both expire entries after a TTL, so a cache whose staleness would be visible for long belongs in the distributed manager instead. Always pair `@Cacheable` with explicit eviction — `@CacheEvict` on the writer service, or a Hibernate `PostUpdateEventListener` / `PostDeleteEventListener`. See `TitleCacheEvictionService` for the canonical pattern, and `PerNodeCacheEvictionService` for propagating a per-node eviction across the cluster. - The bar for adding a new cache: a measured performance gain that justifies the eviction-correctness work. The default answer is: do not cache. - Full rationale, history, and patterns: `documentation/docs/developer/guidelines/caching.mdx`. diff --git a/skills/server-arch-gates/reference/gates.md b/skills/server-arch-gates/reference/gates.md index 4c4661dcf161..e139520b633d 100644 --- a/skills/server-arch-gates/reference/gates.md +++ b/skills/server-arch-gates/reference/gates.md @@ -66,15 +66,29 @@ bypass its invalidation, and because there is no service-level `@Transactional` place to coordinate eviction within a REST call. Both produced cross-node stale-read bugs. **What to use instead.** Spring `@Cacheable`, which resolves against the `RoutingCacheManager` in -`src/main/java/de/tum/cit/aet/artemis/core/config/cache/CacheManagerConfiguration.java`. That serves -blob caches (`files`, `plantUmlPng`, `plantUmlSvg`) from a bounded per-node Caffeine cache and -everything else from the distributed data provider. +`src/main/java/de/tum/cit/aet/artemis/core/config/cache/CacheManagerConfiguration.java`. It routes +each cache to one of two managers: + +- **Per-node Caffeine**, for the blob caches named in `BLOB_CACHE_NAMES` + (`src/main/java/de/tum/cit/aet/artemis/core/config/cache/BlobCacheConfiguration.java`: `files`, + `plantUmlPng`, `plantUmlSvg`) and the title caches named in `TITLE_CACHE_NAMES` + (`src/main/java/de/tum/cit/aet/artemis/core/config/cache/TitleCacheConfiguration.java`). +- **The distributed data provider**, for everything else. + +Every per-node cache also expires entries after a time-to-live. That TTL is the price of moving a +cache off the shared store, and it is the deciding question when you add one: if staleness would be +visible for long, the cache belongs in the distributed manager instead. + +**Cache records, not entities.** A cached Hibernate entity carries its proxies and its association +graph with it. Cache a DTO or a projection. **Always pair it with explicit eviction.** Either `@CacheEvict` on the writing service, or a Hibernate `PostUpdateEventListener` / `PostDeleteEventListener`. The canonical patterns are -`src/main/java/de/tum/cit/aet/artemis/core/service/TitleCacheEvictionService.java` and, for evicting -a per-node blob cache across the cluster, -`src/main/java/de/tum/cit/aet/artemis/core/service/cache/BlobCacheEvictionService.java`. +`src/main/java/de/tum/cit/aet/artemis/core/service/TitleCacheEvictionService.java` and, for +propagating the eviction of a per-node entry to every node, +`src/main/java/de/tum/cit/aet/artemis/core/service/cache/PerNodeCacheEvictionService.java`. The +latter broadcasts over a plain topic on purpose: a dropped broadcast self-corrects within the TTL, +so the retention cost of a reliable topic buys nothing. **The bar.** A measured performance gain that justifies the eviction-correctness work. The default answer is: do not cache. Full rationale and history: diff --git a/supporting_scripts/check_skill_references.py b/supporting_scripts/check_skill_references.py index 5d4c678e9b4e..fc9239d74962 100644 --- a/supporting_scripts/check_skill_references.py +++ b/supporting_scripts/check_skill_references.py @@ -52,8 +52,9 @@ BACKTICK = re.compile(r"`([^`\n]+)`") # Fenced blocks carry the example commands. Their content has no backticks, so they need -# their own pass. -FENCE = re.compile(r"^\s*```") +# their own pass. Markdown allows either fence character, so accept both rather than silently +# skipping a tilde-fenced block. +FENCE = re.compile(r"^\s*(?:```|~~~)") # Trailing punctuation that belongs to the sentence rather than to the path. TRAILING_PUNCTUATION = ".,:;)]}" From b7356a9cb02d4e7e92a3ac6128da9448ee426b1b Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 5 Sep 2026 08:35:13 +0200 Subject: [PATCH 6/9] Development: Close fenced code blocks only with their own delimiter The fence parser used a single generic toggle, so a `~~~` line inside a ``` block closed it, and a ``` line inside a `~~~` block did the same. That inverts the inside/outside state for the rest of the file: real citations in later code blocks stop being checked, and prose starts being scanned as code. The failure is silent, which is the worst shape for a check whose whole job is to notice staleness. Fence matching now follows CommonMark: a block is closed only by a fence of the same character, at least as long as the opening one, and carrying no info string. Anything else is content. Measured against a mixed-delimiter fixture, the old parser missed a citation the new one catches: old: before-infostring, four-backtick-block, inside-tilde-block new: before-infostring, four-backtick-block, inside-tilde-block, inside-backtick-block Those cases are now a --self-test in the script, run as its own CI step before the scan. This repository has no pytest setup, and adding one for a single script would cost more than it returns, so the regression cases live next to the code they guard. Verified that --self-test fails against the old parser and passes against the new one. --- .github/workflows/ci-skills.yml | 5 + documentation/docs/developer/work-with-ai.mdx | 1 + skills/README.md | 5 +- supporting_scripts/check_skill_references.py | 100 ++++++++++++++++-- 4 files changed, 99 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci-skills.yml b/.github/workflows/ci-skills.yml index ced15b3aa7d2..70c52afea0f9 100644 --- a/.github/workflows/ci-skills.yml +++ b/.github/workflows/ci-skills.yml @@ -31,7 +31,12 @@ jobs: uses: actions/setup-python@v6 with: python-version: '3.14' + # The fence state machine decides which lines count as code, so a regression there silently + # stops the check below from seeing example commands. Guard it before trusting the scan. + - name: Checker self-test + run: python supporting_scripts/check_skill_references.py --self-test # A skill that cites a file which has since moved is worse than no skill: an agent acts on it # without checking. This is what keeps the citations honest as the code around them changes. - name: Skill path references + if: ${{ !cancelled() }} run: python supporting_scripts/check_skill_references.py diff --git a/documentation/docs/developer/work-with-ai.mdx b/documentation/docs/developer/work-with-ai.mdx index 9da8dd5a9e5e..e158aceed776 100644 --- a/documentation/docs/developer/work-with-ai.mdx +++ b/documentation/docs/developer/work-with-ai.mdx @@ -166,6 +166,7 @@ Before opening a pull request: ```bash claude --plugin-dir . python3 supporting_scripts/check_skill_references.py +python3 supporting_scripts/check_skill_references.py --self-test claude plugin validate . ``` diff --git a/skills/README.md b/skills/README.md index 1bed46b60f88..552ffcf2c568 100644 --- a/skills/README.md +++ b/skills/README.md @@ -63,8 +63,9 @@ Rules for this repository: Test a skill before opening a pull request: ```bash -claude --plugin-dir . # loads the working copy -python3 supporting_scripts/check_skill_references.py +claude --plugin-dir . # loads the working copy +python3 supporting_scripts/check_skill_references.py # citations still resolve +python3 supporting_scripts/check_skill_references.py --self-test # the checker itself still works claude plugin validate . ``` diff --git a/supporting_scripts/check_skill_references.py b/supporting_scripts/check_skill_references.py index fc9239d74962..d82a8764a7c6 100644 --- a/supporting_scripts/check_skill_references.py +++ b/supporting_scripts/check_skill_references.py @@ -9,6 +9,8 @@ It reads every Markdown file under skills/ and collects path-shaped tokens from two places: inline code spans (single backticks) and the contents of fenced code blocks, where the example commands live. A stale path in an example command is the one that does the most damage, so both are scanned. +Fences may use either delimiter, and a block is closed only by its own: `--self-test` covers the +mixed-delimiter cases, because getting that wrong stops the scan silently rather than loudly. Four kinds of citation are resolved: @@ -37,6 +39,7 @@ Usage: python3 supporting_scripts/check_skill_references.py [--skills-dir skills] + python3 supporting_scripts/check_skill_references.py --self-test """ from __future__ import annotations @@ -51,10 +54,12 @@ # keeps prose such as `@Transactional` or `--specs` out of the check without a list of exceptions. BACKTICK = re.compile(r"`([^`\n]+)`") -# Fenced blocks carry the example commands. Their content has no backticks, so they need -# their own pass. Markdown allows either fence character, so accept both rather than silently -# skipping a tilde-fenced block. -FENCE = re.compile(r"^\s*(?:```|~~~)") +# Fenced blocks carry the example commands. Their content has no backticks, so they need their own +# pass. Both fence characters are accepted, and the delimiter is captured because a block is closed +# only by its own character: a `~~~` line inside a ``` block is content, not a terminator. Getting +# that wrong inverts the inside/outside state for the rest of the file, which would silently skip +# real citations and scan prose as if it were code. +FENCE = re.compile(r"^ {0,3}(?P`{3,}|~{3,})(?P.*)$") # Trailing punctuation that belongs to the sentence rather than to the path. TRAILING_PUNCTUATION = ".,:;)]}" @@ -106,14 +111,28 @@ def tracked_top_level_names(root: Path) -> set[str]: def code_block_tokens(text: str) -> list[str]: - """Whitespace-separated tokens from inside fenced code blocks, stripped of shell noise.""" + """Whitespace-separated tokens from inside fenced code blocks, stripped of shell noise. + + Fence matching follows CommonMark: a block is closed only by a fence of the same character, + at least as long as the opening one, and carrying no info string. Anything else is content. + """ tokens: list[str] = [] - inside = False + open_fence: tuple[str, int] | None = None for line in text.splitlines(): - if FENCE.match(line): - inside = not inside - continue - if not inside: + match = FENCE.match(line) + if match: + delimiter = match.group("delimiter") + if open_fence is None: + # An opening fence may carry an info string, as in "```bash". + open_fence = (delimiter[0], len(delimiter)) + continue + character, length = open_fence + closes = delimiter[0] == character and len(delimiter) >= length + if closes and not match.group("info").strip(): + open_fence = None + continue + # A fence of the other character, or a shorter one, is ordinary content of this block. + if open_fence is None: continue for word in line.split(): tokens.append(word.strip(SURROUNDING_NOISE).rstrip(TRAILING_PUNCTUATION)) @@ -173,6 +192,59 @@ def path_exists(base: Path, token: str, root: Path) -> bool: return candidate.exists() +# The fence state machine is the subtle part of this script: getting it wrong inverts the +# inside/outside state for the rest of a file and silently stops checking real citations. There is +# no pytest setup in this repository, so the regression cases live here and run in CI via +# --self-test rather than pulling in a test framework for one script. +SELF_TEST_DOCUMENT = """\ +```bash +~~~ +./inside-backtick-block.sh +``` + +~~~bash +``` +./inside-tilde-block.sh +~~~ + +````bash +./inside-four-backtick-block.sh +```` + +```bash +./before-a-closing-fence.sh +``` + +./outside-every-block.sh +""" + +SELF_TEST_EXPECTED = { + # A tilde line does not close a backtick block, so this stays inside and is scanned. + "./inside-backtick-block.sh", + # ...and a backtick line does not close a tilde block. + "./inside-tilde-block.sh", + # A longer fence opens and closes normally. + "./inside-four-backtick-block.sh", + "./before-a-closing-fence.sh", + # "./outside-every-block.sh" is prose: absent from the expected set on purpose. +} + + +def self_test() -> int: + """Exercise the fence state machine against mixed delimiters. Returns a process exit code.""" + found = {token for token in code_block_tokens(SELF_TEST_DOCUMENT) if token.startswith("./")} + missing = SELF_TEST_EXPECTED - found + unexpected = found - SELF_TEST_EXPECTED + for token in sorted(missing): + print(f"FAIL: {token} should have been read from inside a fence", file=sys.stderr) + for token in sorted(unexpected): + print(f"FAIL: {token} is outside every fence and was read anyway", file=sys.stderr) + if missing or unexpected: + return 1 + print(f"OK: fence self-test passed ({len(SELF_TEST_EXPECTED)} cases).") + return 0 + + def main() -> int: """Scan the skills directory and report every cited repository path that no longer exists.""" parser = argparse.ArgumentParser( @@ -184,8 +256,16 @@ def main() -> int: default="skills", help="Directory holding the skills (default: skills)", ) + parser.add_argument( + "--self-test", + action="store_true", + help="Run the fence-parsing regression cases instead of scanning the skills", + ) args = parser.parse_args() + if args.self_test: + return self_test() + root = repository_root() skills_dir = (root / args.skills_dir).resolve() From 1aca75a9e5381d5e47c04036986bfd7c00da7fa5 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 5 Sep 2026 08:38:47 +0200 Subject: [PATCH 7/9] Development: State the cloning and EntityManager rules as the build enforces them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review nitpicks, both correct on checking. client-conventions framed the cloning ban as applying to "anything entity-like". The linter makes no such distinction: prefer-deep-clone flags every object spread, Object.assign and structuredClone in production client TypeScript, spec files exempt. Verified with a probe file — a spread of `{ a: 1, b: 2 }` fails exactly like a spread of a Course. The entity-like reasoning is why the rule exists, not what it checks, and the skill now says both. Also records that eslint.config.mjs blocks cloneDeepWith and the lodash-es/cloneDeep subpath, not just cloneDeep, and folds a duplicated array-spread note into one place. server-arch-gates stated "no injected EntityManager or EntityManagerFactory" absolutely while separately naming TitleCacheEvictionService as the canonical eviction pattern. That class is on the rule's exception list precisely because it holds an EntityManagerFactory, to reach the Hibernate EventListenerRegistry and register itself. Read as written, the skill pointed at an example that appears to break the rule it had just stated. Both places now say the list is grandfathering with a TODO attached, name all three classes on it, and tell the reader to copy the eviction logic rather than the constructor. --- skills/client-conventions/SKILL.md | 28 ++++++++++++++------- skills/server-arch-gates/SKILL.md | 7 ++++++ skills/server-arch-gates/reference/gates.md | 13 ++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/skills/client-conventions/SKILL.md b/skills/client-conventions/SKILL.md index 7cf074c7c514..258015860de3 100644 --- a/skills/client-conventions/SKILL.md +++ b/skills/client-conventions/SKILL.md @@ -51,24 +51,34 @@ Use `@if`, `@for`, `@switch`. Never `*ngIf`, `*ngFor`, `*ngSwitch`. ## Copying objects Use `deepClone` from `src/main/webapp/app/foundation/util/deep-clone.util.ts`. Never object spread, -`Object.assign`, or `structuredClone`, for anything entity-like: anything that may hold a `dayjs` -date, a nested object, a `Map` or `Set`, or a circular reference. +`Object.assign`, or `structuredClone`. + +**The ban is unconditional, not a judgement call.** +`localRules/prefer-deep-clone` (`rules/prefer-deep-clone.mjs`) flags every object spread, +`Object.assign` and `structuredClone` in production client TypeScript, spec files exempt. It does +not inspect what the value holds, so `{ ...{ a: 1 } }` fails lint exactly like a spread of a +`Course`. Do not reach for a spread because the object "looks plain". + +The reasoning behind it is about entity-like values, which is where the silent corruption happens: - `structuredClone()` is the worst option. It does not preserve prototypes, so a cloned `dayjs` - date comes back as a plain object with no methods. + date comes back as a plain object with no methods, while `dayjs.isDayjs()` still returns `true`, + so no guard catches it. - Spread and `Object.assign` copy one level. Nested objects stay shared, so a later edit mutates - both. + both. A non-empty `Object.assign` target is mutated in place, which emits no signal notification + because a signal compares with `Object.is`. Two companions live in the same file: `cloneWith(x, { a, b })` replaces `{ ...x, a, b }`, and `hydrate(new Course(), dto)` replaces `Object.assign(new Course(), dto)` for giving a parsed server DTO its prototype. -Enforced by `localRules/prefer-deep-clone` (`rules/prefer-deep-clone.mjs`), production client -TypeScript only, specs exempt. Importing `cloneDeep` from `lodash-es` is blocked so all copying -goes through the wrappers. +Reaching for lodash directly is blocked too: `eslint.config.mjs` forbids importing `cloneDeep` and +`cloneDeepWith` from `lodash-es`, and the `lodash-es/cloneDeep` subpath, so all copying goes +through the wrappers. -Array spread stays fine: `items.update((items) => [...items, newItem])` is the documented way to -append immutably. Object rest in destructuring is fine too. +**Array spread and object rest stay legal.** The rule does not touch them: +`items.update((items) => [...items, newItem])` is the documented way to append immutably, and +`const { a, ...rest } = post` is fine. The signal interaction is subtle and is the part people get wrong. See the cloning section of `reference/migration-recipes.md`. diff --git a/skills/server-arch-gates/SKILL.md b/skills/server-arch-gates/SKILL.md index f1ae199dfc37..3a7178570fd3 100644 --- a/skills/server-arch-gates/SKILL.md +++ b/skills/server-arch-gates/SKILL.md @@ -55,6 +55,13 @@ with `nativeQuery = true` where there is no entity to name. Enforced by `shouldNotUseEntityManagerDirectly` and `shouldNotUseRawJdbcDirectly` in `src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java`. +Three classes sit on that rule's exception list, carrying a TODO to refactor them away. One of them +is `TitleCacheEvictionService`, which holds an `EntityManagerFactory` purely to reach the Hibernate +`EventListenerRegistry` and register itself as a listener. So when the caching section below calls +it the canonical eviction pattern, copy its eviction logic, not its constructor: a new class doing +the same thing fails the rule, because the list is grandfathering rather than permission. Raw JDBC +has no per-class exceptions at all; only `core.config` may hold a `DataSource`. + **Never touch Hazelcast or Redis directly.** All cross-node state goes through `DistributedDataProvider` in `src/main/java/de/tum/cit/aet/artemis/core/service/distributed/`. Enforced by diff --git a/skills/server-arch-gates/reference/gates.md b/skills/server-arch-gates/reference/gates.md index e139520b633d..e92f30b179e5 100644 --- a/skills/server-arch-gates/reference/gates.md +++ b/skills/server-arch-gates/reference/gates.md @@ -28,6 +28,15 @@ name, write a `@Query` with `nativeQuery = true`. `src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java`. The raw JDBC rule permits `core.config` only. +**The exception list is grandfathering, not permission.** `shouldNotUseEntityManagerDirectly` +excludes three classes and carries a TODO to refactor them away: `RepositoryImpl`, +`CustomPostRepositoryImpl`, and `TitleCacheEvictionService`. The last is the one you are most +likely to read, because it is also the canonical cache-eviction pattern below; it holds an +`EntityManagerFactory` only to reach the Hibernate `EventListenerRegistry` and register itself as a +`PostUpdateEventListener` / `PostDeleteEventListener`. Copy its eviction logic, not its +constructor. A new class taking an `EntityManagerFactory` fails the rule, and adding yourself to +the list is the wrong fix. + ## Distributed data **Rule.** Never use Hazelcast or Redis directly. Everything crossing a node boundary, including the @@ -90,6 +99,10 @@ propagating the eviction of a per-node entry to every node, latter broadcasts over a plain topic on purpose: a dropped broadcast self-corrects within the TTL, so the retention cost of a reliable topic buys nothing. +Read `TitleCacheEvictionService` for the eviction logic, not for how it obtains its listener +registration: its `EntityManagerFactory` is a grandfathered exception, as described under +persistence access above. + **The bar.** A measured performance gain that justifies the eviction-correctness work. The default answer is: do not cache. Full rationale and history: `documentation/docs/developer/guidelines/caching.mdx`. From eec6f5ff00a1818797b5157070e8bb344b1abfc3 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 5 Sep 2026 11:25:21 +0200 Subject: [PATCH 8/9] Development: Say which paths the cloning and Bootstrap rules actually cover Review pointed out that prefer-deep-clone does not reach packages/tum-ui, and was right twice over: eslint.config.mjs scopes it to src/main/webapp/app/**/*.ts, and rules/prefer-deep-clone.mjs separately registers no visitors unless the path contains src/main/webapp/. A probe under packages/tum-ui/src/lib confirms it: neither the spread ban nor the lodash import restriction fires there. The kit is also standalone, importing nothing from app/, so deepClone is not reachable from it in the first place. The section now names the enforced path, says the kit is outside it, and keeps the hazard advice, since a dayjs date is corrupted just the same either way. Auditing the other rules I cite for the same mistake turned up one more. no-bootstrap-classes runs on an explicit allow-list of about two dozen already migrated directories, not the whole client, so a passing lint says nothing about a Bootstrap class in an unmigrated area. Saying "enforced by" without that qualifier invites exactly the wrong conclusion. The other cited rules check out: no-raw-tailwind-color-palette and no-primeng-component-classes cover app and tum-ui HTML, enforce-signal-apis covers both source trees, and prefer-signal-reactivity-over-ngonchanges is as documented. --- skills/client-conventions/SKILL.md | 35 +++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/skills/client-conventions/SKILL.md b/skills/client-conventions/SKILL.md index 258015860de3..5ee235f4e030 100644 --- a/skills/client-conventions/SKILL.md +++ b/skills/client-conventions/SKILL.md @@ -53,11 +53,14 @@ Use `@if`, `@for`, `@switch`. Never `*ngIf`, `*ngFor`, `*ngSwitch`. Use `deepClone` from `src/main/webapp/app/foundation/util/deep-clone.util.ts`. Never object spread, `Object.assign`, or `structuredClone`. -**The ban is unconditional, not a judgement call.** -`localRules/prefer-deep-clone` (`rules/prefer-deep-clone.mjs`) flags every object spread, -`Object.assign` and `structuredClone` in production client TypeScript, spec files exempt. It does -not inspect what the value holds, so `{ ...{ a: 1 } }` fails lint exactly like a spread of a -`Course`. Do not reach for a spread because the object "looks plain". +**Where it is enforced: `src/main/webapp/app/**/*.ts`, spec files exempt.** That boundary is set +twice, by the `files:` scope in `eslint.config.mjs` and again inside `rules/prefer-deep-clone.mjs`, +which registers no visitors unless the path contains `src/main/webapp/`. + +**Within that scope the ban is unconditional, not a judgement call.** +`localRules/prefer-deep-clone` flags every object spread, `Object.assign` and `structuredClone` +there. It does not inspect what the value holds, so `{ ...{ a: 1 } }` fails lint exactly like a +spread of a `Course`. Do not reach for a spread because the object "looks plain". The reasoning behind it is about entity-like values, which is where the silent corruption happens: @@ -72,9 +75,15 @@ Two companions live in the same file: `cloneWith(x, { a, b })` replaces `{ ...x, `hydrate(new Course(), dto)` replaces `Object.assign(new Course(), dto)` for giving a parsed server DTO its prototype. -Reaching for lodash directly is blocked too: `eslint.config.mjs` forbids importing `cloneDeep` and -`cloneDeepWith` from `lodash-es`, and the `lodash-es/cloneDeep` subpath, so all copying goes -through the wrappers. +Reaching for lodash directly is blocked too, over the same scope: `eslint.config.mjs` forbids +importing `cloneDeep` and `cloneDeepWith` from `lodash-es`, and the `lodash-es/cloneDeep` subpath, +so all copying goes through the wrappers. + +**`packages/tum-ui` is outside both.** Neither the rule nor the lodash restriction fires there, and +the package is standalone: it imports nothing from `app/`, so `deepClone` is not reachable from it +either. Nothing enforces this section inside the kit. The hazards are unchanged though, so a +component that copies a `dayjs` date or a nested object still needs a deep copy; it just has to +bring its own rather than reach across the package boundary. **Array spread and object rest stay legal.** The rule does not touch them: `items.update((items) => [...items, newItem])` is the documented way to append immutably, and @@ -91,8 +100,14 @@ ng-bootstrap in new work. Colours use semantic tokens. Use TUM UI component variants, or `text-state-danger`, `text-state-success`, `text-state-warning`, `text-state-info` for plain markup. Never `--p--N` primitives, never `text-red-500`, never `text-danger`, never the superseded arbitrary -`text-(--danger)` form. Enforced by `localRules/no-raw-tailwind-color-palette` and -`localRules/no-bootstrap-classes`. +`text-(--danger)` form. + +`localRules/no-raw-tailwind-color-palette` enforces the palette part across +`src/main/webapp/app/**/*.html` and `packages/tum-ui/src/lib/**/*.html`. **The Bootstrap ban is only partly enforced**: +`localRules/no-bootstrap-classes` runs on an explicit allow-list of roughly two dozen already +migrated directories in `eslint.config.mjs`, not on the whole client. Lint passing is therefore not +evidence that a Bootstrap class is acceptable in an unmigrated area; the convention still applies +everywhere, the rule has just not caught up. If you migrate a directory, add it to that list. Never hand-write PrimeNG root classes such as `class="p-button"` or `class="p-inputtext"`. Render the real PrimeNG component so its styles load deterministically. Enforced by From 82a554f1bb6b34538f21f21f5a826f170cbaecf1 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Sat, 5 Sep 2026 11:27:26 +0200 Subject: [PATCH 9/9] Development: Fix the broken inline code span around the gh pr create example The PR-title guidance shows the command inside a single-backtick span whose content itself contains backticks: `gh pr create --title '`Development`: Improve documentation'` Markdown closes the span at the first inner backtick, so it renders as the code fragment "gh pr create --title '" followed by loose text and a stray backtick, rather than as one copyable command. The double-backtick form the same line already uses for the title itself fixes it. Came in from #13654 through the develop merge rather than from this branch, but the line is a few lines above the PR conventions this PR edits, and the fix is one character. --- CLAUDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6b42082abbab..10c39e68e2e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -302,7 +302,7 @@ Organized by feature module: ## Commit & PR Guidelines - Concise, imperative commit messages scoped where useful (e.g. Exam mode: adjust live updates, build: bump version); wrap bodies near 72 chars. Commit messages contain no backticks -- **A PR title wraps the module name in literal backticks and follows it with a colon:** ``​`Development`: Improve documentation``. Only the module before the colon is wrapped, never the whole title and never the text after the colon. The backticks are characters in the title rather than markdown, so quote the title with single quotes so the shell leaves them alone: `gh pr create --title '`Development`: Improve documentation'`. The allowed module names and the exact pattern live in `.github/workflows/validate-pr-title.yml`, and `validate-pr-title` fails the PR when the title does not match. Do not infer the format from `git log`: GitHub strips the backticks when it squashes, so merged subjects read `Development: ...` without them +- **A PR title wraps the module name in literal backticks and follows it with a colon:** ``​`Development`: Improve documentation``. Only the module before the colon is wrapped, never the whole title and never the text after the colon. The backticks are characters in the title rather than markdown, so quote the title with single quotes so the shell leaves them alone: ``gh pr create --title '`Development`: Improve documentation'``. The allowed module names and the exact pattern live in `.github/workflows/validate-pr-title.yml`, and `validate-pr-title` fails the PR when the title does not match. Do not infer the format from `git log`: GitHub strips the backticks when it squashes, so merged subjects read `Development: ...` without them - PRs: include problem/solution summary, linked issue, commands/tests run, screenshots for UI, and doc updates if relevant - Target `develop` branch; rebase to reduce noise - Run lint and tests before submitting