feat(hooks): fold hooks into Save Configuration

[dngrtech]

Summary

• Fold LD_PRELOAD hook enable/order into the shared Save Configuration flow.
• Sync user-hooks/ during the general config-save playbook and guarantee the local rsync source dir exists.
• Remove the old user-facing PUT /instances/<id>/hooks apply route and saveInstanceHooks client call while keeping system-hook task paths intact.
• Enforce hook restart semantics in both frontend and backend: running hook changes restart; stopped hook-only changes stay stopped.
• Update user/API/technical docs and bump version files to 1.13.7.

Test Plan

• .venv/bin/pytest tests/test_sync_configs_playbook_hooks.py tests/test_instance_api_routes.py tests/test_instance_hooks_routes.py tests/test_instance_hooks_files.py tests/test_task_apply_config.py -q → 85 passed
• pnpm --dir frontend-react exec vitest run src/components/instances/__tests__/EditInstanceConfigModal.test.jsx src/components/instances/__tests__/HooksTab.test.jsx src/services/__tests__/api.test.js → 38 passed
• pnpm --dir frontend-react exec eslint src/components/instances/EditInstanceConfigModal.jsx src/components/instances/HooksTab.jsx src/components/instances/__tests__/EditInstanceConfigModal.test.jsx src/components/instances/__tests__/HooksTab.test.jsx src/services/api.js src/services/__tests__/api.test.js
• .venv/bin/pytest tests/test_preset_download_routes.py tests/test_preset_import_routes.py tests/test_preset_import_validation.py tests/test_preset_routes.py tests/test_draft_routes.py -q → 176 passed
• pnpm --dir frontend-react exec vitest run src/services/__tests__/api.test.js src/components/presetManager/__tests__/PresetManagerModal.test.jsx src/components/presetManager/__tests__/PresetLoadTab.test.jsx src/components/presetManager/__tests__/PresetSaveTab.test.jsx src/utils/__tests__/presetDownload.test.js → 27 passed
• Manual preset ZIP roundtrip with user-hooks/custom_hook.so + enabled_hooks.json: download ZIP contained both; import preserved hook file and enabled list.
• python3 -m compileall -q ui/routes/instance_routes.py ui/tasks.py ui/task_logic/ansible_instance_mgmt.py tests/test_instance_api_routes.py tests/test_task_apply_config.py
• git diff --check origin/main...HEAD

Notes

• Broad pnpm lint still has unrelated pre-existing repo errors; changed frontend files pass targeted eslint.
• Plain pytest in this shell points at the Hermes venv and lacks repo deps like redis; backend verification uses .venv/bin/pytest for this repo.

[github-actions]

PR Review — v1.13.7: Hooks save through Save Configuration

Summary of Changes

Removes the dedicated PUT /instances/<id>/hooks + "Apply & Restart" button, merging hook selection state into the existing PUT /instances/<id>/config save flow. HooksTab is refactored from a self-contained stateful component to a controlled component. The Ansible sync_instance_configs_and_restart.yml playbook is extended to sync the user-hooks/ directory to the game host.

---

Strengths

• Clean state lift (HooksTab → EditInstanceConfigModal): eliminates the dual-save inconsistency where hooks and config could be saved independently, leaving the instance in an incoherent state. frontend-react/src/components/instances/HooksTab.jsx
• Correct restart semantics: running instances with hook changes are force-restarted; stopped instances stay stopped. Both the backend route (ui/routes/instance_routes.py:1068–1093) and the frontend toggle (EditInstanceConfigModal.jsx:474–481) enforce this symmetrically.
• previous_status passthrough fixes a latent bug: previously, apply_instance_config_logic read instance.status inside the task, which would already be CONFIGURING (the route sets it before enqueuing). The restored-status logic (set final status to STOPPED when restart=False and was stopped) never worked correctly before this PR. ui/task_logic/ansible_instance_mgmt.py:2022–2025, ui/tasks.py:2038
• hookLoadSeqRef cancellation pattern prevents stale state from concurrent or cancelled hook fetches. EditInstanceConfigModal.jsx:342–390
• os.makedirs(instance_user_hooks_dir, exist_ok=True) prevents rsync failures on legacy instances that never had a user-hooks/ directory. ui/routes/instance_routes.py:1063
• Rsync filter (--include=*.so --exclude=*) limits the sync to only .so files, so stray files on the game host are not clobbered. ansible/playbooks/sync_instance_configs_and_restart.yml
• Good test coverage: new backend tests directly assert the forced-restart and keep-stopped invariants at the API boundary. tests/test_instance_api_routes.py:1496–1590

---

Issues

Critical (Must Fix)

None identified.

---

Important (Should Fix)

1. Reserved filename protection may be absent on the config save path

tests/test_instance_hooks_routes.py:1638–1665 — the old PUT /hooks route rejected force_rate.so (reserved for the system-hook pipeline) with a 400. The new config path calls _filtered_enabled_hooks(enabled_hooks_data, instance_user_hooks_dir), which silently drops entries that don't exist in user-hooks/ but does not appear to check RESERVED_HOOK_FILENAMES. If a user can get force_rate.so into their user-hooks/ directory (e.g., via a rename through PATCH /hooks/files/<filename>), they could shadow the system hook. Verify that either: (a) the upload/rename endpoints reject reserved names, or (b) _filtered_enabled_hooks excludes them. If neither holds, add a reserved-name filter in the config route.

2. confirmDelete does not await onRefresh

frontend-react/src/components/instances/HooksTab.jsx:62–87:

await deleteInstanceHook(instanceId, pendingDelete.filename);
setPendingDelete(null);
onRefresh?.();   // fire-and-forget — no await

onRefresh calls loadHooks(), which is async. If the hook list refresh is slow and the user immediately clicks Save Configuration, the payload will contain a stale hookEnabledOrder that may still include the just-deleted hook. The backend will filter it out, so no data corruption occurs, but the frontend will show a stale order until the refresh resolves. Awaiting onRefresh and holding off on closing the modal until it completes would be cleaner:

setPendingDelete(null);
await onRefresh?.();

3. hooksDirty / hookDiskChanged is not reset after a successful save

EditInstanceConfigModal.jsx:468–473 — hookDiskChanged is set to true when a hook binary is replaced (upload over an existing enabled hook) and is never cleared. If Save Configuration succeeds but the modal remains open (e.g., on a save error retry scenario where the first attempt failed), hooksDirty will remain true, and running instances will keep the restart toggle forced on even though the last save was already applied. The fix is to reset hookDiskChanged (and re-seed initialHookEnabledOrder) after a successful save, just as other tabs reset their dirty state.

---

Minor (Nice to Have)

4. hooksKeepStopped doesn't restore the restart toggle when the user reverts hook changes

EditInstanceConfigModal.jsx:479–481:

if (hooksForceRestart) setRestartAfterSave(true);
if (hooksKeepStopped) setRestartAfterSave(false);

When hooksKeepStopped becomes false (user reverts all hook changes), restartAfterSave stays at false. The toggle is re-enabled (since restartForced becomes false), so the user can manually re-enable it, but the visual state is surprising. Consider storing the pre-force value and restoring it on revert.

5. test_put_hooks_route_removed skips auth

tests/test_instance_hooks_routes.py:1683–1685:

def test_put_hooks_route_removed(client):
    resp = client.put('/api/instances/1/hooks', json={'enabled': []})
    assert resp.status_code == 405

The test doesn't send an auth header and passes because 405 is returned before auth. This is acceptable but note that it tests HTTP routing, not security. Consider adding a complementary test that sends valid auth and confirms 405 (versus a hypothetical 401 masking the route removal).

6. Ansible: no --checksum flag on hook rsync

ansible/playbooks/sync_instance_configs_and_restart.yml:24–34 — rsync defaults to mtime+size comparison. Replaced .so files with identical size but different content (e.g., a patched hook binary re-uploaded at the same size) may not be re-synced if the game host mtime happens to match. Adding --checksum removes this edge case at the cost of a slightly slower sync. Low priority given how .so files are typically managed, but worth noting.

7. enabled_hooks silently drops invalid filenames rather than warning

The old PUT /hooks returned 400 for each invalid or missing entry, giving the client explicit feedback. The new path (_filtered_enabled_hooks) silently drops non-existent entries. This is documented in the API reference and is intentional, but callers have no way to tell whether their hook list was truncated. A response field like "dropped_hooks": ["stale.so"] in the 202 body would aid debugging.

---

Assessment

Ready to merge? Yes, with the Important issues tracked.

Reasoning: The core refactor is architecturally sound and fixes a real usability pain point. The previous_status fix corrects a latent bug that prevented stopped-instance semantics from working correctly. The Important issues (reserved filename bypass risk, fire-and-forget refresh on delete, dirty-state not reset post-save) are all low-severity in practice given the upload-time validation and backend filtering, but should be addressed in a follow-up to keep the defenses layered correctly.

[dngrtech]

Fresh Review

Verdict

REQUEST_CHANGES

Findings

Important

• Replacing an already-enabled hook binary is not tracked as a hook change, so a running instance can save without the required restart and without syncing the new binary as an intentional dirty change.
  • Evidence:
    • frontend-react/src/components/instances/HookRow.jsx:231-238 calls replaceInstanceHook(...) and then only onChanged?.() with no indication that hook bytes changed.
    • frontend-react/src/components/instances/HooksTab.jsx:141-159 passes onRefresh straight through as onChanged for enabled and disabled rows.
    • frontend-react/src/components/instances/EditInstanceConfigModal.jsx:743-745 only sets hookDiskChanged when options.hooksChanged is truthy.
    • ui/routes/instance_routes.py:1051-1079 only forces restart when the stored enabled hook list changes; replacing the same enabled filename leaves next_ld_preload_hooks == original_ld_preload_hooks, so a client-provided restart: false is honored.
  • Impact: For a running instance, replacing a.so while a.so is enabled leaves hooksDirty === false because the enabled order is unchanged. The UI may not force the restart toggle, and even an API client can save the unchanged enabled_hooks list with restart: false. The new .so is not loaded until a later manual restart, violating the “running hook changes must restart” requirement.
  • Recommended fix: propagate hook file replacement as an explicit hook change, e.g. onChanged?.({ hooksChanged: hook.enabled }), and include a hook_files_changed/similar flag in the Save Configuration payload so the backend treats it like hooks_changed for restart semantics. Add coverage for replacing an enabled hook on a running instance and for the backend honoring that file-change signal even when the enabled filename list is unchanged.

Verification

• git status --short && git diff --stat origin/main...HEAD && git diff --check origin/main...HEAD
  • Working tree clean; diff stat inspected; git diff --check reported no whitespace errors.
• git grep -n "saveInstanceHooks\|put_instance_hooks\|enqueue_apply_hooks\|PUT /api/instances/<id>/hooks" -- . ':!node_modules' || true
  • No stale references found.
• Reviewed backend/frontend/playbook/docs diffs for the hook save flow, route removal, restart semantics, preset/user-hooks behavior, and version docs.
• .venv/bin/pytest tests/test_instance_api_routes.py tests/test_instance_hooks_routes.py tests/test_sync_configs_playbook_hooks.py tests/test_task_apply_config.py -q
  • 59 passed, 57 warnings in 8.87s
• cd frontend-react && pnpm test -- --run src/components/instances/__tests__/EditInstanceConfigModal.test.jsx src/components/instances/__tests__/HooksTab.test.jsx src/services/__tests__/api.test.js
  • Vitest completed successfully; output showed 43 passed (43) test files and 218 passed (218) tests, with existing console/act/polyfill warnings.

Notes

• The overall fold into Save Configuration is close: route removal, client removal, user-hooks sync in the shared playbook, previous-status passing, preset hook roundtrip, docs, and version bump are all present.
• Reviewed by a fresh Hermes subagent; parent verified the reported file/line evidence before posting.

[dngrtech]

Addressed in 97ca9b9

Fixed the one confirmed issue: replacing an already-enabled hook binary now forces a restart.

Root cause

The per-row Replace handlers in HookRow.jsx (HookRowContent.onReplaceFile and HookActionsMenu.onReplaceFile) called onChanged?.() with no args, so handleRefreshHooks's options.hooksChanged was undefined and setHookDiskChanged(true) never fired. Replacing an enabled hook left hooksDirty === false, so the restart toggle was not forced — a running instance could save with restart off and keep the old .so loaded. The sibling upload path (HooksTab.handleUpload) already did this correctly via { hooksChanged: enabledSet.has(file.name) }.

Fix

Both replace handlers now propagate onChanged?.({ hooksChanged: hook.enabled }), mirroring the upload path. hook.enabled is exactly the right condition — replacing a disabled hook's bytes doesn't require a restart.

On the backend hook_files_changed flag (not changed)

Deferring this as an optional follow-up. It defends the raw API boundary against a client that replaces a binary then sends restart: false — but in this single-user app that client is the admin, and the frontend fix closes the realistic UI path (the modal now always sends restart: true for this case). Not worth adding a new payload field + backend branch to this refactor.

[dngrtech]

Test coverage added in f13bac9 (HooksTab.test.jsx):

• replacing an ENABLED hook binary → asserts onRefresh is called with { hooksChanged: true } (forces restart).
• replacing a DISABLED hook binary → asserts { hooksChanged: false } (no restart).

Both drive the real HookRow through HooksTab. Verified they fail against the pre-fix onChanged?.() and pass with onChanged?.({ hooksChanged: hook.enabled }). Full frontend suite green (220 passed).