# Code Review Report: Feature 26 — Mesh Node Metadata Registry

This report performs a deep-dive audit of the `AgentNode` and `NodeGroupAccess` models in `node.py`, focusing on **Mesh Inventory Scaling**, **ACL Integrity**, and **Foreign Key Performance**.

---

## 🏗️ 12-Factor App Compliance Audit

| Factor | Status | Observation |
| :--- | :--- | :--- |
| **VI. Processes** | ✅ **Success** | **Distributed Heartbeat Storage**: The `last_seen_at` and `last_status` columns (Line 21, 22) provide a unified "Single Source of Truth" for mesh health that survives Hub process cycles—enabling sub-second detection of offline nodes upon system reboot. |
| **IV. Backing Services** | 🔴 **Major Risk** | **String-Key Referential Hazard**: The `NodeGroupAccess` model uses `node_id` (a mutable String) as its foreign key (Line 35) rather than the immutable integer `id`. If a node is renamed in the registry, all associated Access Control lists will break referential integrity unless an explicit "ON UPDATE CASCADE" is managed manually at the application layer. |

---

## 🔍 File-by-File Diagnostic

### 1. `app/db/models/node.py`
The persistent registry for mesh nodes, their capabilities, and access permissions.

> [!TIP]
> **Performance: Composite ACL Hits**
> Line 31: `NodeGroupAccess` table.
> For large clusters with complex organizational policies, resolving "Which groups have access to this node?" will occur on almost every gRPC handshake and shell command dispatch.
> 
> **Recommendation**: Implement a unique composite constraint on `(node_id, group_id)` to prevent redundant permission records and enable index-only scans for ACL resolution.

**Identified Problems**:
*   **JSON Default Mutation**: The nested `skill_config` default (Line 14) is a dictionary. While safe in SQLAlchemy for new records, SQLAlchemy's "Change Tracking" for nested JSON mutations is notoriously inconsistent without using the `MutableDict` helper—potentially leading to silented "Save Failures" during partial config updates.
*   **Orphaned Node Cleanup**: While `GroupAccess` has `delete-orphan` (Line 26), there is no cascade from `User` (the `registered_by` field). If an Admin user is deleted, their registered nodes will remain orphaned in the database.

---

## 🛠️ Summary Recommendations

1.  **Switch to Surrogate Foreign Keys**: Transition `NodeGroupAccess` to use the integer `id` of `AgentNode` to ensure that node identifiers can be safely renamed or re-formatted without breaking ACLs.
2.  **Enable Mutable Change Tracking**: Utilize `sqlalchemy.ext.mutable.MutableDict` for the `skill_config` and `capabilities` columns to ensure that nested dictionary updates are correctly detected and committed to the database.
3.  **Enforce ACL Constraints**: Add a `UniqueConstraint('node_id', 'group_id')` to the `NodeGroupAccess` table to maintain sub-second permission evaluation and prevent record duplication.

---

**This concludes Feature 26. I have persisted this report to `/app/docs/reviews/feature_review_node_metadata.md`. Shall I proceed to audit the final Asset model?**