From 976b3f0005cb560cee70f1cccb45ee569e3cf83d Mon Sep 17 00:00:00 2001
From: hermes <ridgway_infrastructure@gmail.com>
Date: Tue, 29 Sep 2026 16:22:25 +1000
Subject: [PATCH] docs: record the review follow-ups, with evidence for the one
 we declined

Second review round asked for three things. Two adopted, one measured and declined.

ADOPTED
  - Removing the Terminal plugin is recommended in writing as the right move (it is the
    largest attack surface, and macros already provide command execution). Left installed
    because it is the operator's plugin - removing a tool unilaterally is not the agent's
    call. Documented how: rename to Terminal.disabled (readDirectory() skips *.disabled).
  - Automatic password rotation and the deploy-key-read-only suggestion are recorded as
    operator decisions with their trade-offs.

DECLINED, WITH EVIDENCE
  - "php.ini should disable exec/shell_exec/passthru/system by default." Measured, this
    would break the editor's core function, so it cannot be a default:
        components/codegit/*   -> Common::safe_execute   31 call sites (every git op)
        components/macro/*     -> Common::raw_execute     1 call site
        common.php raw_execute -> function_exists("system") / function_exists("exec")
    Every commit, push and fetch from the editor runs through exec/system. It stays a
    deliberate exposure lever (S-7), not a default.

CORRECTED
  - Review claimed the redacted patch file had a syntax error. Verified on the guest with
    php83 -l: "No syntax errors detected". The placeholder is a plain string literal inside
    a double-quoted PHP string. That finding was wrong, and is recorded as such rather than
    "fixed" to satisfy a false positive.

Also fixed the section ordering in the doc.
---
 docs/known-issues.md | 50 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/docs/known-issues.md b/docs/known-issues.md
index 3b01a03..a516b75 100644
--- a/docs/known-issues.md
+++ b/docs/known-issues.md
@@ -170,3 +170,53 @@ grep -E 'PHP Fatal' /var/log/nginx/atheos.error.log | tail
 
 **Rule of thumb:** if a plugin was ported from Codiad, assume its `Common::` calls are stale and
 verify them before trusting it.
+
+---
+
+## 4. Follow-ups not done (recorded so they are not forgotten)
+
+These came out of review. They are **operator decisions**, not agent ones — several change what the
+editor can do.
+
+### 4.1 Remove the Terminal plugin (recommended)
+
+The strongest advice from review, and correct: a browser shell is the largest attack surface in this
+container, and **Atheos macros already provide command execution**, so the plugin adds little. It was
+installed during testing to diagnose a plugin-rendering problem. Removing the plugin directory (or
+renaming it to `Terminal.disabled` — `readDirectory()` skips `*.disabled`) removes the surface.
+
+Left installed because it is the **operator's** plugin and removing it unilaterally would take away a
+tool without being asked.
+
+### 4.2 ❌ `disable_functions` cannot be "on by default" here — measured
+
+Review asked for `php.ini` to disable `exec`, `shell_exec`, `passthru`, `system` by default.
+**This would break the editor's core function.** Evidence:
+
+```
+components/codegit/*      → Common::safe_execute  (31 call sites — every git operation)
+components/macro/*        → Common::raw_execute   (1 call site)
+common.php raw_execute    → function_exists("system") / function_exists("exec")
+```
+
+Every git commit, push, fetch and status from the editor goes through `exec`/`system`. Disabling them
+turns the editor into a text editor that cannot commit. So it stays **a deliberate exposure lever**
+(S-7 in `docs/security-notes.md`), to be applied only when the container is exposed — at which point
+you would also lose macros and git integration, which is why S-7 already says *"test, do not
+blind-apply"*.
+
+### 4.3 Rotate the terminal password automatically
+
+If the plugin is kept, `provision.sh` should generate a random password rather than leaving the
+upstream default. Not implemented: it would hardcode a secret into a repo file, which is the same
+class of problem this file already documents.
+
+### 4.4 Deploy key could be read-only
+
+Review suggested making the deploy key read-only if writes are unnecessary. **They are necessary** —
+that is the whole point of the writer-branch workflow (macro 2 pushes). Not applicable.
+
+### 4.5 The real upstream fix for §2
+
+`class.macro.php::save()` should write `[]` instead of `"N/A"` for folder macros. See §2 — the fix is
+currently applied to the data, so it survives updates, at the cost of being reversible from the UI.