mirror of
https://github.com/jokob-sk/NetAlertX.git
synced 2026-09-18 18:47:00 -04:00
DOCS+BE: skill updates, write_notifications fix #1793
This commit is contained in:
jokob-sk committed
1 parent
45e86b5646
commit
52d1698221
12 files changed
+188
-5
No files matched your search
@@ -0,0 +1,28 @@
|
||||
---
|
||||
name: install-scripts
|
||||
description: Read before editing anything under install/ (production-filesystem/, ubuntu24/, etc.) or reviewing a PR that touches it. Covers uninstall safety, why check-*.sh scripts don't all exit the same way, and what's legacy vs. active.
|
||||
---
|
||||
|
||||
# Install Scripts
|
||||
|
||||
Conventions mined from real review comments on `install/*` PRs (#1214, #1230, #1235) — not derived from reading the scripts alone, so check current behavior before relying on any of these if the surrounding code has since changed.
|
||||
|
||||
## Uninstall safety
|
||||
|
||||
`install/*/uninstall.sh` must only remove NetAlertX's own files (`/app` and its own data) — never touch system packages it depends on (PHP, nginx, avahi, etc.) or other shared system components. A hardware/bare-metal install shares the host with other software; removing a system package on uninstall can break things NetAlertX didn't install.
|
||||
|
||||
## `check-*.sh` scripts don't all exit the same way — by design
|
||||
|
||||
`install/production-filesystem/services/scripts/check-*.sh` scripts are **not uniform** in whether a failure is fatal:
|
||||
- Some are fail-fast on purpose: `check-first-run-config.sh` exits non-zero on a critical failure (can't create the config dir, can't copy default config) so the container doesn't start in a broken state.
|
||||
- Some are warning-only on purpose: `check-ramdisk.sh` exits 0 even when it detects a real issue, letting the app start anyway with suboptimal config.
|
||||
|
||||
Don't assume one check's exit-code convention applies to another — check what that specific script is actually guarding before suggesting a change to its exit behavior.
|
||||
|
||||
## `/back` is legacy, not the active code path
|
||||
|
||||
`back/cron_script.sh` (and the rest of `/back`) is legacy, kept only for compatibility with older/external components. The active, maintained version is `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` are out of scope for most PRs unless the PR is specifically about compatibility with whatever still depends on it.
|
||||
|
||||
## Build-time vs. runtime, and generated files
|
||||
|
||||
See the `devcontainer-configs`/`devcontainer-management` skills for the Dockerfile-generation and build-vs-runtime-file-availability rules — the same "source files aren't present at Docker build time" constraint applies to `install/production-filesystem/build/*` scripts, not just the devcontainer image.
|
||||
@@ -5,6 +5,18 @@ description: How to analyze and respond to GitHub PR review comments in NetAlert
|
||||
|
||||
# PR Analysis
|
||||
|
||||
## Standing Rule: A Repeated Comment Becomes a Skill Update
|
||||
|
||||
jokob (the maintainer) has asked for this explicitly on real PRs, not just implied it:
|
||||
- PR #1739 (test-mocking/MAC-case feedback repeated across two PRs): "will those skill prevent the issues I had to comment on for you to fix in the last 2 PRs? ... will you next time follow these based on available guidelines and skills? if not update those to make this explicit"
|
||||
- PR #1744 (wrong `mylog()` level): "change 'warn' to 'none' and update skills so this doesn't happen again"
|
||||
|
||||
So: if a review comment corrects something a skill *should* already cover, don't just fix that one instance — update the relevant skill as part of addressing the comment, same PR, same turn. If no skill covers it yet, that's the signal to create one. Check the skill first, though — sometimes it already covers the point and just wasn't consulted (e.g. `logging-standards` already lists the valid `mylog()` levels; the PR #1744 fix was in *applying* it, not in the skill being incomplete).
|
||||
|
||||
## Code Style (from real review history)
|
||||
|
||||
- **Prefer explicit, readable logic over compact-but-opaque expressions.** Real feedback on a `max()`-based one-liner: "make it easier to read, using max() obfuscates logic." When a built-in/clever expression saves a line or two but a reader has to reverse-engineer *why* it produces the right answer, write it out as plain conditional logic instead.
|
||||
|
||||
## Before Writing Any Test Code — Non-Negotiable Checklist
|
||||
|
||||
Run through this before creating or editing any file under `test/`:
|
||||
|
||||
@@ -0,0 +1,28 @@
|
||||
---
|
||||
name: install-scripts
|
||||
description: Read before editing anything under install/ (production-filesystem/, ubuntu24/, etc.) or reviewing a PR that touches it. Covers uninstall safety, why check-*.sh scripts don't all exit the same way, and what's legacy vs. active.
|
||||
---
|
||||
|
||||
# Install Scripts
|
||||
|
||||
Conventions mined from real review comments on `install/*` PRs (#1214, #1230, #1235) — not derived from reading the scripts alone, so check current behavior before relying on any of these if the surrounding code has since changed.
|
||||
|
||||
## Uninstall safety
|
||||
|
||||
`install/*/uninstall.sh` must only remove NetAlertX's own files (`/app` and its own data) — never touch system packages it depends on (PHP, nginx, avahi, etc.) or other shared system components. A hardware/bare-metal install shares the host with other software; removing a system package on uninstall can break things NetAlertX didn't install.
|
||||
|
||||
## `check-*.sh` scripts don't all exit the same way — by design
|
||||
|
||||
`install/production-filesystem/services/scripts/check-*.sh` scripts are **not uniform** in whether a failure is fatal:
|
||||
- Some are fail-fast on purpose: `check-first-run-config.sh` exits non-zero on a critical failure (can't create the config dir, can't copy default config) so the container doesn't start in a broken state.
|
||||
- Some are warning-only on purpose: `check-ramdisk.sh` exits 0 even when it detects a real issue, letting the app start anyway with suboptimal config.
|
||||
|
||||
Don't assume one check's exit-code convention applies to another — check what that specific script is actually guarding before suggesting a change to its exit behavior.
|
||||
|
||||
## `/back` is legacy, not the active code path
|
||||
|
||||
`back/cron_script.sh` (and the rest of `/back`) is legacy, kept only for compatibility with older/external components. The active, maintained version is `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` are out of scope for most PRs unless the PR is specifically about compatibility with whatever still depends on it.
|
||||
|
||||
## Build-time vs. runtime, and generated files
|
||||
|
||||
See the `devcontainer-configs`/`devcontainer-management` skills for the Dockerfile-generation and build-vs-runtime-file-availability rules — the same "source files aren't present at Docker build time" constraint applies to `install/production-filesystem/build/*` scripts, not just the devcontainer image.
|
||||
@@ -5,6 +5,18 @@ description: How to analyze and respond to GitHub PR review comments in NetAlert
|
||||
|
||||
# PR Analysis
|
||||
|
||||
## Standing Rule: A Repeated Comment Becomes a Skill Update
|
||||
|
||||
jokob (the maintainer) has asked for this explicitly on real PRs, not just implied it:
|
||||
- PR #1739 (test-mocking/MAC-case feedback repeated across two PRs): "will those skill prevent the issues I had to comment on for you to fix in the last 2 PRs? ... will you next time follow these based on available guidelines and skills? if not update those to make this explicit"
|
||||
- PR #1744 (wrong `mylog()` level): "change 'warn' to 'none' and update skills so this doesn't happen again"
|
||||
|
||||
So: if a review comment corrects something a skill *should* already cover, don't just fix that one instance — update the relevant skill as part of addressing the comment, same PR, same turn. If no skill covers it yet, that's the signal to create one. Check the skill first, though — sometimes it already covers the point and just wasn't consulted (e.g. `logging-standards` already lists the valid `mylog()` levels; the PR #1744 fix was in *applying* it, not in the skill being incomplete).
|
||||
|
||||
## Code Style (from real review history)
|
||||
|
||||
- **Prefer explicit, readable logic over compact-but-opaque expressions.** Real feedback on a `max()`-based one-liner: "make it easier to read, using max() obfuscates logic." When a built-in/clever expression saves a line or two but a reader has to reverse-engineer *why* it produces the right answer, write it out as plain conditional logic instead.
|
||||
|
||||
## Before Writing Any Test Code — Non-Negotiable Checklist
|
||||
|
||||
Run through this before creating or editing any file under `test/`:
|
||||
|
||||
@@ -25,8 +25,9 @@ Skills with the same purpose exist in more than one, sometimes under different n
|
||||
| Project navigation | `project-navigation` | `project-navigation` | — | Copilot version has full path tables and env vars; Gemini version is a brief reference |
|
||||
| Plugin dev | `plugin-development` | `plugin-run-development` | `plugin-development` | All three cover data contract, phases, formats, the `RUN_TIMEOUT` kill-timer gotcha (`timeoutMultiplier`/`per_item_timeout()`), and a pre-PR pointer to the Conventions Checklist in `docs/PLUGINS_DEV.md` |
|
||||
| Plugin README docs | `plugin-readme` | `plugin-readme` | `plugin-readme` | All three cover README structure, the "don't re-document settings" rule, the `docs.netalertx.com` cross-linking convention, and common defects (template leftovers, copy-paste errors) found during a full-repo audit |
|
||||
| Devcontainer | `devcontainer-management` | `devcontainer-services` + `devcontainer-setup` + `devcontainer-configs` | — | Gemini combines into one (uses `docker exec`); Copilot splits into 3 focused skills |
|
||||
| PR review | `pr-analysis` | `pr-analysis` | `pr-analysis` | How to classify and respond to PR comments; pre-flight skill loading checklist |
|
||||
| Devcontainer | `devcontainer-management` | `devcontainer-services` + `devcontainer-setup` + `devcontainer-configs` | — | Gemini combines into one (uses `docker exec`); Copilot splits into 3 focused skills. `devcontainer-configs` also covers Dockerfile generation (root `Dockerfile` → `generate-dockerfile.sh` → `.devcontainer/Dockerfile`, source files not present at build time) - mined from real review comments on PR #1184/#1230. |
|
||||
| Install scripts | `install-scripts` | `install-scripts` | `install-scripts` | `install/*` conventions mined from real review comments (#1214/#1230/#1235): uninstall must only remove NetAlertX's own files, never system packages; `check-*.sh` scripts intentionally differ on fail-fast vs. warning-only; `/back` is legacy, not the active code path. |
|
||||
| PR review | `pr-analysis` | `pr-analysis` | `pr-analysis` | How to classify and respond to PR comments; pre-flight skill loading checklist; the standing rule that a repeated review comment becomes a skill update (sourced from jokob's own real PR comments, #1739/#1744); a readability-over-cleverness code-style note from the same source. |
|
||||
| Logging | `logging-standards` | `logging-standards` | — | `mylog` levels, message format, what not to log |
|
||||
| Scan pipeline internals | `scan-pipeline` | `scan-pipeline` | `scan-pipeline` | `process_scan()` call order and why it's load-bearing, `CurrentScan`/`Events`/`Sessions`/`DevicesView` relationships, how a session actually closes (no `close_session()` exists), and the `FIELD_SPECS` field-write authority mechanism. Complements `database-patterns` (Devices write-path/`*Source` attribution) rather than duplicating it. |
|
||||
| Database patterns | `database-patterns` | `database-patterns` | `database-patterns` | Devices table write-path inventory, the `FIELD_SOURCE_MAP`/`*Source` attribution system in `server/db/authoritative_handler.py`, SQLite trigger vs. Python-hook tradeoffs, and event-sourced vs. snapshot audit logging. |
|
||||
|
||||
@@ -26,3 +26,11 @@ Combines and merges template configurations into the final config used by VS Cod
|
||||
## Note
|
||||
|
||||
This affects only the devcontainer configuration. It has no bearing on the production or test Docker image.
|
||||
|
||||
## Dockerfile Generation (separate from the config generation above)
|
||||
|
||||
`.devcontainer/Dockerfile` is itself a **generated file** — `generate-dockerfile.sh` combines the root `Dockerfile` with `.devcontainer/resources/devcontainer-Dockerfile`. Never review or edit `.devcontainer/Dockerfile` directly; dependency/build-step changes belong in the root `Dockerfile`, regenerate after.
|
||||
|
||||
Two build-time gotchas that have caused real review confusion:
|
||||
- **Source repo files are not available during the Docker build phase** — they're mounted into the container after it starts. A `COPY` referencing a source file will fail at build time; config that depends on repo content has to be handled at runtime (a setup script), not a build step.
|
||||
- Under modern BuildKit, `COPY` with a glob pattern that matches nothing (e.g. `.[V]ERSION`) succeeds silently — copies nothing, doesn't fail the build. Don't assume a missing optional file will surface as a build error.
|
||||
@@ -0,0 +1,28 @@
|
||||
---
|
||||
name: netalertx-install-scripts
|
||||
description: Read before editing anything under install/ (production-filesystem/, ubuntu24/, etc.) or reviewing a PR that touches it. Covers uninstall safety, why check-*.sh scripts don't all exit the same way, and what's legacy vs. active.
|
||||
---
|
||||
|
||||
# Install Scripts
|
||||
|
||||
Conventions mined from real review comments on `install/*` PRs (#1214, #1230, #1235) — not derived from reading the scripts alone, so check current behavior before relying on any of these if the surrounding code has since changed.
|
||||
|
||||
## Uninstall safety
|
||||
|
||||
`install/*/uninstall.sh` must only remove NetAlertX's own files (`/app` and its own data) — never touch system packages it depends on (PHP, nginx, avahi, etc.) or other shared system components. A hardware/bare-metal install shares the host with other software; removing a system package on uninstall can break things NetAlertX didn't install.
|
||||
|
||||
## `check-*.sh` scripts don't all exit the same way — by design
|
||||
|
||||
`install/production-filesystem/services/scripts/check-*.sh` scripts are **not uniform** in whether a failure is fatal:
|
||||
- Some are fail-fast on purpose: `check-first-run-config.sh` exits non-zero on a critical failure (can't create the config dir, can't copy default config) so the container doesn't start in a broken state.
|
||||
- Some are warning-only on purpose: `check-ramdisk.sh` exits 0 even when it detects a real issue, letting the app start anyway with suboptimal config.
|
||||
|
||||
Don't assume one check's exit-code convention applies to another — check what that specific script is actually guarding before suggesting a change to its exit behavior.
|
||||
|
||||
## `/back` is legacy, not the active code path
|
||||
|
||||
`back/cron_script.sh` (and the rest of `/back`) is legacy, kept only for compatibility with older/external components. The active, maintained version is `install/production-filesystem/services/scripts/cron_script.sh`. Changes to `/back` are out of scope for most PRs unless the PR is specifically about compatibility with whatever still depends on it.
|
||||
|
||||
## Build-time vs. runtime, and generated files
|
||||
|
||||
See the `devcontainer-configs`/`devcontainer-management` skills for the Dockerfile-generation and build-vs-runtime-file-availability rules — the same "source files aren't present at Docker build time" constraint applies to `install/production-filesystem/build/*` scripts, not just the devcontainer image.
|
||||
@@ -5,6 +5,18 @@ description: How to analyze and respond to GitHub PR review comments in NetAlert
|
||||
|
||||
# PR Analysis
|
||||
|
||||
## Standing Rule: A Repeated Comment Becomes a Skill Update
|
||||
|
||||
jokob (the maintainer) has asked for this explicitly on real PRs, not just implied it:
|
||||
- PR #1739 (test-mocking/MAC-case feedback repeated across two PRs): "will those skill prevent the issues I had to comment on for you to fix in the last 2 PRs? ... will you next time follow these based on available guidelines and skills? if not update those to make this explicit"
|
||||
- PR #1744 (wrong `mylog()` level): "change 'warn' to 'none' and update skills so this doesn't happen again"
|
||||
|
||||
So: if a review comment corrects something a skill *should* already cover, don't just fix that one instance — update the relevant skill as part of addressing the comment, same PR, same turn. If no skill covers it yet, that's the signal to create one. Check the skill first, though — sometimes it already covers the point and just wasn't consulted (e.g. `logging-standards` already lists the valid `mylog()` levels; the PR #1744 fix was in *applying* it, not in the skill being incomplete).
|
||||
|
||||
## Code Style (from real review history)
|
||||
|
||||
- **Prefer explicit, readable logic over compact-but-opaque expressions.** Real feedback on a `max()`-based one-liner: "make it easier to read, using max() obfuscates logic." When a built-in/clever expression saves a line or two but a reader has to reverse-engineer *why* it produces the right answer, write it out as plain conditional logic instead.
|
||||
|
||||
## Before Writing Any Test Code — Non-Negotiable Checklist
|
||||
|
||||
Run through this before creating or editing any file under `test/`:
|
||||
|
||||
@@ -25,8 +25,9 @@ Skills with the same purpose exist in more than one, sometimes under different n
|
||||
| Project navigation | `project-navigation` | `project-navigation` | — | Copilot version has full path tables and env vars; Gemini version is a brief reference |
|
||||
| Plugin dev | `plugin-run-development` | `plugin-development` | `plugin-development` | All three cover data contract, phases, formats, the `RUN_TIMEOUT` kill-timer gotcha (`timeoutMultiplier`/`per_item_timeout()`), and a pre-PR pointer to the Conventions Checklist in `docs/PLUGINS_DEV.md` |
|
||||
| Plugin README docs | `plugin-readme` | `plugin-readme` | `plugin-readme` | All three cover README structure, the "don't re-document settings" rule, the `docs.netalertx.com` cross-linking convention, and common defects (template leftovers, copy-paste errors) found during a full-repo audit |
|
||||
| Devcontainer | `devcontainer-services` + `devcontainer-setup` + `devcontainer-configs` | `devcontainer-management` | — | Copilot splits into 3 focused skills; Gemini combines into one (uses `docker exec`) |
|
||||
| PR review | `pr-analysis` | `pr-analysis` | `pr-analysis` | How to classify and respond to PR comments; pre-flight skill loading checklist |
|
||||
| Devcontainer | `devcontainer-services` + `devcontainer-setup` + `devcontainer-configs` | `devcontainer-management` | — | Copilot splits into 3 focused skills; Gemini combines into one (uses `docker exec`). `devcontainer-configs` also covers Dockerfile generation (root `Dockerfile` → `generate-dockerfile.sh` → `.devcontainer/Dockerfile`, source files not present at build time) - mined from real review comments on PR #1184/#1230. |
|
||||
| Install scripts | `install-scripts` | `install-scripts` | `install-scripts` | `install/*` conventions mined from real review comments (#1214/#1230/#1235): uninstall must only remove NetAlertX's own files, never system packages; `check-*.sh` scripts intentionally differ on fail-fast vs. warning-only; `/back` is legacy, not the active code path. |
|
||||
| PR review | `pr-analysis` | `pr-analysis` | `pr-analysis` | How to classify and respond to PR comments; pre-flight skill loading checklist; the standing rule that a repeated review comment becomes a skill update (sourced from jokob's own real PR comments, #1739/#1744); a readability-over-cleverness code-style note from the same source. |
|
||||
| Logging | `logging-standards` | `logging-standards` | — | `mylog` levels, message format, what not to log |
|
||||
| Scan pipeline internals | `scan-pipeline` | `scan-pipeline` | `scan-pipeline` | `process_scan()` call order and why it's load-bearing, `CurrentScan`/`Events`/`Sessions`/`DevicesView` relationships, how a session actually closes (no `close_session()` exists), and the `FIELD_SPECS` field-write authority mechanism. Complements `database-patterns` (Devices write-path/`*Source` attribution) rather than duplicating it. |
|
||||
| Database patterns | `database-patterns` | `database-patterns` | `database-patterns` | Devices table write-path inventory, the `FIELD_SOURCE_MAP`/`*Source` attribution system in `server/db/authoritative_handler.py`, SQLite trigger vs. Python-hook tradeoffs, and event-sourced vs. snapshot audit logging. |
|
||||
|
||||
@@ -34,6 +34,7 @@ GROUPS = [
|
||||
[".gemini/skills/ux-design-patterns/SKILL.md", ".github/skills/ux-design-patterns/SKILL.md", ".claude/skills/ux-design-patterns/SKILL.md"],
|
||||
[".gemini/skills/skill-hygiene/SKILL.md", ".github/skills/skill-hygiene/SKILL.md", ".claude/skills/skill-hygiene/SKILL.md"],
|
||||
[".gemini/skills/plugin-review/SKILL.md", ".github/skills/plugin-review/SKILL.md", ".claude/skills/plugin-review/SKILL.md"],
|
||||
[".gemini/skills/install-scripts/SKILL.md", ".github/skills/install-scripts/SKILL.md", ".claude/skills/install-scripts/SKILL.md"],
|
||||
[".gemini/skills/settings/SKILL.md", ".github/skills/settings-management/SKILL.md"],
|
||||
[".gemini/skills/mcp-activation/SKILL.md", ".github/skills/mcp-activation/SKILL.md"],
|
||||
[".gemini/skills/project-navigation/SKILL.md", ".github/skills/project-navigation/SKILL.md"],
|
||||
|
||||
@@ -912,7 +912,7 @@ class CreateNotificationRequest(BaseModel):
|
||||
content: str = Field(
|
||||
...,
|
||||
min_length=1,
|
||||
max_length=1024,
|
||||
max_length=4096,
|
||||
description="Notification content"
|
||||
)
|
||||
level: NOTIFICATION_LEVELS = Field(
|
||||
@@ -920,6 +920,18 @@ class CreateNotificationRequest(BaseModel):
|
||||
description="Notification severity level"
|
||||
)
|
||||
|
||||
@field_validator("content", mode="before")
|
||||
@classmethod
|
||||
def truncate_content(cls, v):
|
||||
"""Truncate instead of rejecting - content is often a generated
|
||||
summary (e.g. a bulk multi-edit's affected MAC list) with no natural
|
||||
length cap, and the notification store is a JSON file with no
|
||||
underlying column limit that 4096 corresponds to. Losing the whole
|
||||
write on overlength input would be worse than a truncated one."""
|
||||
if isinstance(v, str) and len(v) > 4096:
|
||||
return v[:4093] + "..."
|
||||
return v
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# SYNC SCHEMAS
|
||||
|
||||
@@ -107,6 +107,46 @@ def test_delete_single_notification(client, api_token, notification_guid):
|
||||
assert resp.json.get("success") is True
|
||||
|
||||
|
||||
def test_write_notification_truncates_overlong_content(client, api_token):
|
||||
"""A notification whose content exceeds 4096 chars (e.g. a bulk multi-edit's
|
||||
generated MAC-list summary, selecting hundreds of devices) must be
|
||||
truncated, not rejected outright - silently dropping the whole
|
||||
notification loses the audit trail of what the bulk action did."""
|
||||
overlong = "[Multi edit] Executed \"update\" matching \"" + ",".join(
|
||||
f"{i:04x}:{i:04x}:{i:04x}" for i in range(300)
|
||||
) + "\""
|
||||
assert len(overlong) > 4096
|
||||
|
||||
resp = client.post(
|
||||
"/messaging/in-app/write",
|
||||
json={"content": overlong, "level": "info"},
|
||||
headers=auth_headers(api_token)
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert resp.json.get("success") is True
|
||||
|
||||
resp = client.get("/messaging/in-app/unread", headers=auth_headers(api_token))
|
||||
stored = next((n["content"] for n in resp.json if n["content"].startswith("[Multi edit]")), None)
|
||||
assert stored is not None
|
||||
assert len(stored) == 4096
|
||||
assert stored.endswith("...")
|
||||
assert stored.startswith(overlong[:100])
|
||||
|
||||
|
||||
def test_write_notification_at_exact_limit_unchanged(client, api_token):
|
||||
exactly_4096 = "x" * 4096
|
||||
resp = client.post(
|
||||
"/messaging/in-app/write",
|
||||
json={"content": exactly_4096, "level": "info"},
|
||||
headers=auth_headers(api_token)
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
|
||||
resp = client.get("/messaging/in-app/unread", headers=auth_headers(api_token))
|
||||
stored = next((n["content"] for n in resp.json if n["content"].startswith("xxx")), None)
|
||||
assert stored == exactly_4096 # untouched - no trailing "..."
|
||||
|
||||
|
||||
def test_delete_all_notifications(client, api_token, random_content):
|
||||
# Add a notification first
|
||||
client.post("/messaging/in-app/write", json={"content": random_content}, headers=auth_headers(api_token))
|
||||
|
||||
Reference in new issue
Block a user