celilo/celilo

Fork 0

Give module hooks a writable directory — today anything they write fails `module audit` as `extra` #1000

New issue

Open

opened 2026-08-20 18:29:25 +00:00 by forgejo-admin · 5 comments

forgejo-admin commented

2026-08-20 18:29:25 +00:00

Owner

A module's install root is checksummed, and the integrity audit reports every file it did not expect:

// apps/celilo/src/module/packaging/audit.ts:198-210
const actualFiles = await scanDirectory(moduleDir, moduleDir);
const expectedFiles = new Set(Object.keys(expectedChecksums));
for (const file of actualFiles) {
  if (!expectedFiles.has(file)) {
    violations.push({ type: 'extra', path: file, message: `Unexpected file: ${file}` });
  }
}

success is violations.length === 0, so one such file fails the audit.

But hooks do write into their own root, and so does the framework on their behalf. packages/capabilities/src/browser.ts:237 is celilo's own code doing it:

return join(modulePath, 'screenshots', safeKey);

The only reason that does not fail today is that classifyModulePath carries a hardcoded allow-list of the specific paths someone has already been bitten by:

// apps/celilo/src/module/packaging/audit.ts (classifyModulePath)
if (segments[0] === 'generated' || segments[0] === 'screenshots') return 'derived';
if (relPath === 'checksums.json' || relPath === 'signature.sig') return 'derived';
if (relPath === 'celilo/types.d.ts') return 'derived';
if (relPath === 'cookies.json') return 'derived';
return 'package';   // <- everything else gets an integrity claim

screenshots/ and cookies.json are two patches for one capability. The default for anything else is package, so the next hook that writes a cache file, a cursor, a lock, or a scratch artifact next to itself gets Unexpected file: and a red audit — with no hint that writing there was the mistake, because nothing says where it should have written instead.

That is whack-a-mole with a hardcoded list, and the list lives in celilo core while the writers live in modules and capabilities. It also makes the audit's own contract unclear: extra is supposed to mean "something is in this install that we did not put there", which is a real security property, and it currently also means "a hook did its job".

celilo already has the pattern for the fix. BROWSER_ROOT = '/var/lib/celilo/browsers' (packages/capabilities/src/browser.ts:28) is a celilo-owned tree outside any module directory. A per-module equivalent — a writable directory the framework creates, injects into HookContext, and classifies derived wholesale — would give hooks somewhere to write that is safe by construction rather than by allow-list entry.

Open questions for whoever picks this up, rather than a decided design:

One directory per module (<module>/var/, classified derived like generated/) or a celilo-owned tree outside the module (/var/lib/celilo/module-state/<id>/)? The first keeps everything about a module in one place and travels with backup's existing per-module walk. The second keeps the install root genuinely immutable, which is a stronger integrity claim.
Should screenshots/ and cookies.json migrate into it and the allow-list entries be deleted, or stay as compatibility?
Does it need to survive module update? generated/ does; a scratch cache might not want to.

Scope

apps/celilo/src/module/packaging/audit.ts — classifyModulePath (the allow-list) and the extra check at :198.
packages/capabilities/src/browser.ts:237 — the framework write into modulePath.
packages/capabilities/src/types.ts:83, define-hook.ts:84 — the screenshot-directory surface handed to hooks.
Grep join(modulePath and join(sourcePath across packages/capabilities and modules/*/scripts for other writers.
packages/capabilities/src/browser.ts:28 (BROWSER_ROOT) as the existing precedent for a celilo-owned writable tree.

Acceptance

A hook has a documented, framework-provided place to write that is not its install root.
Writing there does not produce an extra violation, and does so by rule rather than by an entry in a hardcoded list.
extra recovers its real meaning: something is present that celilo did not put there.
screenshots/ and cookies.json either migrate or are documented as deliberate exceptions with a reason.
Recurrence gate: a fixture module whose hook writes a file with a name nobody anticipated, asserting a clean audit. This is the case the current allow-list cannot cover and the one that will recur.

openspec/changes/multi-instance-modules (renaming to submodules) — D4 gives each instance a private root reached through a symlink farm, precisely so N instances' hook writes do not land on top of each other. That change needs this question answered to know where those writes should go, but does not block on it.
celilo#173 — modules run their bundled copy; the install root's integrity is why this matters.

A module's install root is checksummed, and the integrity audit reports every file it did not expect: ```ts // apps/celilo/src/module/packaging/audit.ts:198-210 const actualFiles = await scanDirectory(moduleDir, moduleDir); const expectedFiles = new Set(Object.keys(expectedChecksums)); for (const file of actualFiles) { if (!expectedFiles.has(file)) { violations.push({ type: 'extra', path: file, message: `Unexpected file: ${file}` }); } } ``` `success` is `violations.length === 0`, so one such file fails the audit. But hooks **do** write into their own root, and so does the framework on their behalf. `packages/capabilities/src/browser.ts:237` is celilo's own code doing it: ```ts return join(modulePath, 'screenshots', safeKey); ``` The only reason that does not fail today is that `classifyModulePath` carries a hardcoded allow-list of the specific paths someone has already been bitten by: ```ts // apps/celilo/src/module/packaging/audit.ts (classifyModulePath) if (segments[0] === 'generated' || segments[0] === 'screenshots') return 'derived'; if (relPath === 'checksums.json' || relPath === 'signature.sig') return 'derived'; if (relPath === 'celilo/types.d.ts') return 'derived'; if (relPath === 'cookies.json') return 'derived'; return 'package'; // <- everything else gets an integrity claim ``` `screenshots/` and `cookies.json` are two patches for one capability. The default for anything else is `package`, so the next hook that writes a cache file, a cursor, a lock, or a scratch artifact next to itself gets `Unexpected file:` and a red audit — with no hint that writing there was the mistake, because nothing says where it should have written instead. That is whack-a-mole with a hardcoded list, and the list lives in celilo core while the writers live in modules and capabilities. It also makes the audit's own contract unclear: `extra` is supposed to mean "something is in this install that we did not put there", which is a real security property, and it currently also means "a hook did its job". **celilo already has the pattern for the fix.** `BROWSER_ROOT = '/var/lib/celilo/browsers'` (`packages/capabilities/src/browser.ts:28`) is a celilo-owned tree outside any module directory. A per-module equivalent — a writable directory the framework creates, injects into `HookContext`, and classifies `derived` wholesale — would give hooks somewhere to write that is safe by construction rather than by allow-list entry. Open questions for whoever picks this up, rather than a decided design: - One directory per module (`<module>/var/`, classified derived like `generated/`) or a celilo-owned tree outside the module (`/var/lib/celilo/module-state/<id>/`)? The first keeps everything about a module in one place and travels with backup's existing per-module walk. The second keeps the install root genuinely immutable, which is a stronger integrity claim. - Should `screenshots/` and `cookies.json` migrate into it and the allow-list entries be deleted, or stay as compatibility? - Does it need to survive `module update`? `generated/` does; a scratch cache might not want to. ## Scope - `apps/celilo/src/module/packaging/audit.ts` — `classifyModulePath` (the allow-list) and the `extra` check at `:198`. - `packages/capabilities/src/browser.ts:237` — the framework write into `modulePath`. - `packages/capabilities/src/types.ts:83`, `define-hook.ts:84` — the screenshot-directory surface handed to hooks. - Grep `join(modulePath` and `join(sourcePath` across `packages/capabilities` and `modules/*/scripts` for other writers. - `packages/capabilities/src/browser.ts:28` (`BROWSER_ROOT`) as the existing precedent for a celilo-owned writable tree. ## Acceptance - [ ] A hook has a documented, framework-provided place to write that is not its install root. - [ ] Writing there does not produce an `extra` violation, and does so by rule rather than by an entry in a hardcoded list. - [ ] `extra` recovers its real meaning: something is present that celilo did not put there. - [ ] `screenshots/` and `cookies.json` either migrate or are documented as deliberate exceptions with a reason. - [ ] **Recurrence gate:** a fixture module whose hook writes a file with a name nobody anticipated, asserting a clean audit. This is the case the current allow-list cannot cover and the one that will recur. ## Related - `openspec/changes/multi-instance-modules` (renaming to `submodules`) — D4 gives each instance a private root reached through a symlink farm, precisely so N instances' hook writes do not land on top of each other. That change needs this question answered to know where those writes should go, but does not block on it. - celilo#173 — modules run their bundled copy; the install root's integrity is why this matters.

forgejo-admin added the

severity:medium

category:bug

labels

2026-08-20 18:29:25 +00:00

forgejo-admin commented

2026-08-20 18:49:36 +00:00

Author

Owner

Closing the open question in the body: put it inside the module root as a named state/ directory classified derived. The machinery already exists and only the name is missing.

I was leaning toward a celilo-owned tree outside the module (the BROWSER_ROOT shape), on the argument that it keeps the install root immutable and extra at full strength. module-update.ts says otherwise:

* `derived` is neither walked nor removed. It is celilo's or the operator's —
* `generated/`, the hook runtime closure, `screenshots/`, `cookies.json` — and
* `generated/` alone carries terraform state and provider binaries.

and at :338-345:

Only package-class paths are pruned: generated/, the hook runtime closure, screenshots/ and cookies.json are celilo's or the operator's, and survive an update by design.

So derived already means precisely "writable, survives module update, not audited, not pruned". That is the entire contract a sanctioned scratch location needs, and it is already implemented and already load-bearing. What is missing is not machinery. It is a named directory carrying that classification, instead of two ad-hoc entries (screenshots/, cookies.json) that someone patched in after being bitten.

That collapses the fix to roughly one line in classifyModulePath:

if (segments[0] === 'generated' || segments[0] === 'state') return 'derived';

and deleting the two ad-hoc entries once their writers move.

The integrity objection I was going to raise answers itself. Carving a hole in extra is not a new compromise — the hole exists, it is called derived, and generated/ is a far larger one. A single named directory is that same compromise applied consistently, and it is a net reduction: one rule replacing two unrelated special cases.

Everything the outside-the-module option would have needed — a path helper, HookContext injection, backup integration, an update-lifecycle decision, cleanup on module remove — is work to re-derive semantics derived already provides.

Name: state/, not var/. var collides with celilo's own variables.* manifest vocabulary and would read as "the module's variables" to anyone skimming an install tree.

Two consequences worth carrying into the work:

Backup gets it for free via the existing per-module walk, which is correct for a cursor or a registration record and possibly wrong for a large cache. Worth deciding deliberately rather than discovering at restore.
state/ survives module update by construction, which is what a cursor wants. If some future scratch wants discarding on update, that is a second directory with a different rule, not a reason to reject this one.

**Closing the open question in the body: put it inside the module root as a named `state/` directory classified `derived`. The machinery already exists and only the name is missing.** I was leaning toward a celilo-owned tree outside the module (the `BROWSER_ROOT` shape), on the argument that it keeps the install root immutable and `extra` at full strength. `module-update.ts` says otherwise: ``` * `derived` is neither walked nor removed. It is celilo's or the operator's — * `generated/`, the hook runtime closure, `screenshots/`, `cookies.json` — and * `generated/` alone carries terraform state and provider binaries. ``` and at `:338-345`: > Only `package`-class paths are pruned: `generated/`, the hook runtime closure, `screenshots/` and `cookies.json` are celilo's or the operator's, and **survive an update by design**. So `derived` already means precisely "writable, survives `module update`, not audited, not pruned". That is the entire contract a sanctioned scratch location needs, and it is already implemented and already load-bearing. What is missing is not machinery. It is a **named directory** carrying that classification, instead of two ad-hoc entries (`screenshots/`, `cookies.json`) that someone patched in after being bitten. That collapses the fix to roughly one line in `classifyModulePath`: ```ts if (segments[0] === 'generated' || segments[0] === 'state') return 'derived'; ``` and deleting the two ad-hoc entries once their writers move. The integrity objection I was going to raise answers itself. Carving a hole in `extra` is not a new compromise — the hole exists, it is called `derived`, and `generated/` is a far larger one. A single named directory is that same compromise applied *consistently*, and it is a net reduction: one rule replacing two unrelated special cases. Everything the outside-the-module option would have needed — a path helper, `HookContext` injection, backup integration, an update-lifecycle decision, cleanup on module remove — is work to re-derive semantics `derived` already provides. **Name: `state/`, not `var/`.** `var` collides with celilo's own `variables.*` manifest vocabulary and would read as "the module's variables" to anyone skimming an install tree. Two consequences worth carrying into the work: - **Backup gets it for free** via the existing per-module walk, which is correct for a cursor or a registration record and possibly wrong for a large cache. Worth deciding deliberately rather than discovering at restore. - **`state/` survives `module update` by construction**, which is what a cursor wants. If some future scratch wants discarding on update, that is a second directory with a different rule, not a reason to reject this one.

forgejo-admin commented

2026-08-22 13:12:55 +00:00

Author

Owner

The decision above is not on any branch, and three separate changes are now waiting on it. Recording that here so it is visible from the issue rather than only from the agents blocked by it.

The comment above closes the design question and collapses the fix to roughly one line in classifyModulePath plus deleting two ad-hoc entries once their writers move. Checked today against both origin/main and origin/multi-instance-impl:

git grep -n "=== 'state'" -- apps/celilo/src/module/packaging/package-rules.ts

returns nothing on either. classifyModulePath (package-rules.ts:98-103) still returns derived for generated/, screenshots/, checksums.json, signature.sig, celilo/types.d.ts and cookies.json, and nothing else. The line proposed above is written nowhere. This issue is still open and still unassigned.

Who is waiting.

openspec/changes/hook-process-boundary task 4.2 derives the jail's mount set and needs state/ as its sanctioned writable row. Its task 7.2 says "confirm celilo#1000's state/ has shipped before task 4.2 depends on it. Do not re-decide it and do not touch classifyModulePath" — an instruction that cannot be satisfied by waiting, because nothing is in flight to wait for. Its design D11 states the decision above as settled input.
openspec/changes/capability-owned-tables task 4.7 defers celilo#1018's clientConfig write into the module tree to this decision, on the stated grounds that "a fourth answer to where does a deploy-time artifact live is worse than a late one" (design.md:258). That agent confirms it has never touched classifyModulePath and does not own this.
openspec/changes/multi-instance-modules (submodules) group 8 carries celilo#1000 as a task. Its agent has not started it.

Each of the three has independently declined to own it, and each declined for a defensible reason: re-deciding a decided question, or scattering a fourth answer. So the gap is not neglect, it is three correct local decisions summing to nobody writing the line.

Two things need an operator, and they are different questions.

Ratification. The decision above was made by an agent, not by peba. Forgejo attributes it to forgejo-admin because that is the API token identity every agent writes under, so authorship is not visible from the issue itself. The argument is substantial and reasoned — derived already means "writable, survives module update, not audited, not pruned", so the contract exists and only a name was missing — and nobody who has read it wants to reopen it. It has simply never been ratified. That is cheap to answer and worth answering explicitly, because task 7.2 tells a future implementer not to re-decide it, which is only safe advice if it was decided by someone entitled to.
An owner for the one line, plus the recurrence gate this issue's own acceptance criteria already specify: a fixture module whose hook writes a file with a name nobody anticipated, asserting a clean audit.

One dissent worth recording, from the hook-process-boundary agent, offered as a mild preference and explicitly not as grounds to reopen. Inside the module root means binding <store>/<id> read-only and then binding <store>/<id>/state read-write on top of it, a nested read-write exception carved into a read-only tree. Bubblewrap handles it, since a later --bind wins over an earlier --ro-bind, so it works. But D9's clean claim is "the module's own tree is bound read-only", and a nested hole makes that sentence slightly less true. Outside the root there is no exception at all. It confirms this does not change the mount set either way — one writable row, a different string in it — and that stage 2 can build the mount set now and fill the row in when the shape is fixed.

**The decision above is not on any branch, and three separate changes are now waiting on it.** Recording that here so it is visible from the issue rather than only from the agents blocked by it. The comment above closes the design question and collapses the fix to roughly one line in `classifyModulePath` plus deleting two ad-hoc entries once their writers move. Checked today against both `origin/main` and `origin/multi-instance-impl`: ``` git grep -n "=== 'state'" -- apps/celilo/src/module/packaging/package-rules.ts ``` returns nothing on either. `classifyModulePath` (`package-rules.ts:98-103`) still returns `derived` for `generated/`, `screenshots/`, `checksums.json`, `signature.sig`, `celilo/types.d.ts` and `cookies.json`, and nothing else. The line proposed above is written nowhere. This issue is still open and still unassigned. **Who is waiting.** 1. `openspec/changes/hook-process-boundary` task 4.2 derives the jail's mount set and needs `state/` as its sanctioned writable row. Its task 7.2 says "confirm celilo#1000's `state/` has shipped before task 4.2 depends on it. Do not re-decide it and do not touch `classifyModulePath`" — an instruction that cannot be satisfied by waiting, because nothing is in flight to wait for. Its design D11 states the decision above as settled input. 2. `openspec/changes/capability-owned-tables` task 4.7 defers celilo#1018's `clientConfig` write into the module tree to this decision, on the stated grounds that "a fourth answer to where does a deploy-time artifact live is worse than a late one" (`design.md:258`). That agent confirms it has never touched `classifyModulePath` and does not own this. 3. `openspec/changes/multi-instance-modules` (submodules) group 8 carries celilo#1000 as a task. Its agent has not started it. Each of the three has independently declined to own it, and each declined for a defensible reason: re-deciding a decided question, or scattering a fourth answer. So the gap is not neglect, it is three correct local decisions summing to nobody writing the line. **Two things need an operator, and they are different questions.** - **Ratification.** The decision above was made by an agent, not by peba. Forgejo attributes it to `forgejo-admin` because that is the API token identity every agent writes under, so authorship is not visible from the issue itself. The argument is substantial and reasoned — `derived` already means "writable, survives `module update`, not audited, not pruned", so the contract exists and only a name was missing — and nobody who has read it wants to reopen it. It has simply never been ratified. That is cheap to answer and worth answering explicitly, because task 7.2 tells a future implementer not to re-decide it, which is only safe advice if it was decided by someone entitled to. - **An owner for the one line**, plus the recurrence gate this issue's own acceptance criteria already specify: a fixture module whose hook writes a file with a name nobody anticipated, asserting a clean audit. One dissent worth recording, from the `hook-process-boundary` agent, offered as a mild preference and explicitly not as grounds to reopen. Inside the module root means binding `<store>/<id>` read-only and then binding `<store>/<id>/state` read-write on top of it, a nested read-write exception carved into a read-only tree. Bubblewrap handles it, since a later `--bind` wins over an earlier `--ro-bind`, so it works. But D9's clean claim is "the module's own tree is bound read-only", and a nested hole makes that sentence slightly less true. Outside the root there is no exception at all. It confirms this does not change the mount set either way — one writable row, a different string in it — and that stage 2 can build the mount set now and fill the row in when the shape is fixed.

forgejo-admin commented

2026-08-22 14:51:08 +00:00

Author

Owner

Landed on main in #1091 (package-rules.ts:105), with the recurrence gate at apps/celilo/src/module/packaging/module-state-directory.test.ts.

The location decision in the comment above was ratified by peba before implementation, so it is an operator's call rather than a peer's.

The gate uses generated filenames, not literals, which is the part worth keeping. The failure this issue describes is not "we forgot to allow state/cursor.json" — it is that the allow-list was a list of literals patched in one at a time after each one bit someone, so it could only ever cover names somebody had already been surprised by. A test asserting a literal name would reproduce exactly that weakness.

Watched failing first (Rule 7.6), with the gate written and the line not yet added:

(fail) any name a hook invents under state/ is derived, at any depth
(fail) a hook writing into state/ leaves module audit clean
     +   "path": "state/16-74mu.dat",
     +   "type": "extra",
 1 pass, 2 fail

That "type": "extra" is this issue's defect, reproduced end to end through auditModule against a real tree and a real database rather than synthesised on both sides.

The one test that passed before and after is the contrast, and it is load-bearing: the same generated names outside state/ still classify package, so they are still scanned and still reported. If that ever goes green alongside the others, the fix widened rather than named.

Left open deliberately

screenshots/ and cookies.json are untouched. This issue leaves their migration to when their writers move, and folding it in turns a one-line change into a capability refactor.

Two consequences for other work

The jail gets one nested exception. state/ sits inside the module root, so hook-process-boundary's D9 mount set binds <store>/<id> read-only and then binds <store>/<id>/state read-write on top of it. Bubblewrap resolves that correctly (a later --bind wins over an earlier --ro-bind), but D9's sentence "the module's own tree is bound read-only" now has exactly one carved exception. Recorded in that change's task 4.2 so whoever writes the mount set meets it there.

lunacycle now has a destination it did not have. Its health-check.ts:57 and smoke-handler.ts:49 write post-mortem artifacts to a hardcoded /tmp/lunacycle-smoke on the management host, and health-check.ts:433 prints an operator-facing message naming that directory. Under the jail, /tmp is a fresh tmpfs per run, so the writes would succeed, the hook would report success, and the artifacts would be gone before an operator followed the sentence telling them where to look — with no error anywhere. state/ is the answer to that, and it is a one-line destination change now rather than an open question. Out-of-repo, so it is lunacycle's to make.

Landed on `main` in #1091 (`package-rules.ts:105`), with the recurrence gate at `apps/celilo/src/module/packaging/module-state-directory.test.ts`. The location decision in the comment above was ratified by peba before implementation, so it is an operator's call rather than a peer's. **The gate uses generated filenames, not literals**, which is the part worth keeping. The failure this issue describes is not "we forgot to allow `state/cursor.json`" — it is that the allow-list was a list of literals patched in one at a time after each one bit someone, so it could only ever cover names somebody had already been surprised by. A test asserting a literal name would reproduce exactly that weakness. Watched failing first (Rule 7.6), with the gate written and the line not yet added: ``` (fail) any name a hook invents under state/ is derived, at any depth (fail) a hook writing into state/ leaves module audit clean + "path": "state/16-74mu.dat", + "type": "extra", 1 pass, 2 fail ``` That `"type": "extra"` is this issue's defect, reproduced end to end through `auditModule` against a real tree and a real database rather than synthesised on both sides. The one test that passed before and after is the contrast, and it is load-bearing: the same generated names *outside* `state/` still classify `package`, so they are still scanned and still reported. If that ever goes green alongside the others, the fix widened rather than named. ## Left open deliberately `screenshots/` and `cookies.json` are untouched. This issue leaves their migration to when their writers move, and folding it in turns a one-line change into a capability refactor. ## Two consequences for other work **The jail gets one nested exception.** `state/` sits inside the module root, so `hook-process-boundary`'s D9 mount set binds `<store>/<id>` read-only and then binds `<store>/<id>/state` read-write on top of it. Bubblewrap resolves that correctly (a later `--bind` wins over an earlier `--ro-bind`), but D9's sentence "the module's own tree is bound read-only" now has exactly one carved exception. Recorded in that change's task 4.2 so whoever writes the mount set meets it there. **lunacycle now has a destination it did not have.** Its `health-check.ts:57` and `smoke-handler.ts:49` write post-mortem artifacts to a hardcoded `/tmp/lunacycle-smoke` on the management host, and `health-check.ts:433` prints an operator-facing message naming that directory. Under the jail, `/tmp` is a fresh tmpfs per run, so the writes would succeed, the hook would report success, and the artifacts would be gone before an operator followed the sentence telling them where to look — with no error anywhere. `state/` is the answer to that, and it is a one-line destination change now rather than an open question. Out-of-repo, so it is lunacycle's to make.

forgejo-admin closed this issue

2026-08-22 14:51:12 +00:00

forgejo-admin referenced this issue from celilo/lunacycle

2026-08-22 15:23:07 +00:00

Hooks write smoke artifacts to /tmp/lunacycle-smoke, which the hook jail turns into a lying diagnostic — move them to state/ #64

forgejo-admin referenced this issue from celilo/lunacycle

2026-08-22 15:38:20 +00:00

Hooks write smoke artifacts to /tmp/lunacycle-smoke, which the hook jail turns into a lying diagnostic — move them to state/ #64

forgejo-admin reopened this issue

2026-08-22 15:51:21 +00:00

forgejo-admin commented

2026-08-22 15:51:41 +00:00

Author

Owner

Reopening. I closed this early and the first acceptance criterion is not met.

A hook has a documented, framework-provided place to write that is not its install root.

#1091 shipped the classification and not the surface. classifyModulePath tolerates state/, and the gate proves an unanticipated filename there leaves module audit clean. But HookContext has no stateDir. Grepping origin/main across apps/celilo/src and packages/capabilities/src finds nothing computing <moduleRoot>/state for a hook to receive. packages/capabilities/src/types.ts:84 carries screenshotDir and nothing else of that shape.

So a module author has to construct the path themselves, which is exactly what "framework-provided" was written to prevent.

The tell was in my own test and I did not read it. module-state-directory.test.ts builds the path by hand:

mkdirSync(join(root, 'state'), { recursive: true });
writeFileSync(join(root, 'state', unanticipatedName(42)), 'whatever the hook needed');

If the gate has to hand-roll the path, so does every module. I verified the classification landed and treated that as the issue being done.

Found by the first real consumer, not by review. celilo/lunacycle#64 is moving smoke artifacts out of /tmp/lunacycle-smoke into state/ and is blocked on having nothing to ask. That is the right way for this to surface and the wrong way for it to have been necessary.

What is still owed

stateDir on HookContext, populated the way screenshotDir already is (executor.ts:630 computes it, :641 injects it, :675 creates it with mkdirSync(recursive)).
Created on demand. A module that has never written state has no state/ yet, and a hook should not have to mkdir -p its own sanctioned location.
The line in reference/MODULE_DEVELOPMENT_GUIDE.md that makes "documented" true.

Two decisions recorded here rather than left implicit

A plain path, not an accessor. openspec/changes/hook-owned-state is turning context.secrets and context.config into get/set/delete accessors, which raises the fair question of whether state/ should match. It should not, and the reason is that they are not the same kind of thing. Those accessors mediate values celilo persists in the database. state/ is a directory a module writes files into — a SQLite file, a screenshot, a DOM dump. An accessor over a directory means celilo mediating file I/O, which is the "capability-only filesystem" option hook-process-boundary D8 rejected outright: 15 hook-reachable module files use node:fs, and a path is what they can consume. screenshotDir is the correct sibling.

Per module, long-lived. Not per run. screenshotDir is per-run (moduleArtifactDir(modulePath, ${hookName}-${startTime})) because artifacts are one invocation's diagnostic output and retention prunes them. state/ is the opposite by construction: this issue's whole argument is that derived means "survives module update", which is what a cursor wants. Per-run state is a contradiction.

The consequence, stated so it is a decision and not a surprise: two concurrent hooks for the same module share one directory. That is possible today — a health_check can run while a deploy is in flight, and the bus dispatcher spawns handlers concurrently. celilo will not isolate them, because isolating them defeats the purpose: a cursor written in one run must be readable in the next. A module needing atomicity inside its own storage does what any program does (atomic rename, distinct names, a lock file). This goes in the guide next to the path.

I am taking this.

**Reopening. I closed this early and the first acceptance criterion is not met.** > - [ ] A hook has a documented, framework-provided place to write that is not its install root. #1091 shipped the **classification** and not the **surface**. `classifyModulePath` tolerates `state/`, and the gate proves an unanticipated filename there leaves `module audit` clean. But `HookContext` has no `stateDir`. Grepping `origin/main` across `apps/celilo/src` and `packages/capabilities/src` finds nothing computing `<moduleRoot>/state` for a hook to receive. `packages/capabilities/src/types.ts:84` carries `screenshotDir` and nothing else of that shape. So a module author has to construct the path themselves, which is exactly what "framework-provided" was written to prevent. **The tell was in my own test and I did not read it.** `module-state-directory.test.ts` builds the path by hand: ```ts mkdirSync(join(root, 'state'), { recursive: true }); writeFileSync(join(root, 'state', unanticipatedName(42)), 'whatever the hook needed'); ``` If the gate has to hand-roll the path, so does every module. I verified the classification landed and treated that as the issue being done. **Found by the first real consumer, not by review.** `celilo/lunacycle#64` is moving smoke artifacts out of `/tmp/lunacycle-smoke` into `state/` and is blocked on having nothing to ask. That is the right way for this to surface and the wrong way for it to have been necessary. ## What is still owed - `stateDir` on `HookContext`, populated the way `screenshotDir` already is (`executor.ts:630` computes it, `:641` injects it, `:675` creates it with `mkdirSync(recursive)`). - Created on demand. A module that has never written state has no `state/` yet, and a hook should not have to `mkdir -p` its own sanctioned location. - The line in `reference/MODULE_DEVELOPMENT_GUIDE.md` that makes "documented" true. ## Two decisions recorded here rather than left implicit **A plain path, not an accessor.** `openspec/changes/hook-owned-state` is turning `context.secrets` and `context.config` into `get`/`set`/`delete` accessors, which raises the fair question of whether `state/` should match. It should not, and the reason is that they are not the same kind of thing. Those accessors mediate **values celilo persists in the database**. `state/` is **a directory a module writes files into** — a SQLite file, a screenshot, a DOM dump. An accessor over a directory means celilo mediating file I/O, which is the "capability-only filesystem" option `hook-process-boundary` D8 rejected outright: 15 hook-reachable module files use `node:fs`, and a path is what they can consume. `screenshotDir` is the correct sibling. **Per module, long-lived. Not per run.** `screenshotDir` is per-run (`moduleArtifactDir(modulePath, `${hookName}-${startTime}`)`) because artifacts are one invocation's diagnostic output and retention prunes them. `state/` is the opposite by construction: this issue's whole argument is that `derived` means "survives `module update`", which is what a cursor wants. Per-run state is a contradiction. The consequence, stated so it is a decision and not a surprise: **two concurrent hooks for the same module share one directory.** That is possible today — a `health_check` can run while a deploy is in flight, and the bus dispatcher spawns handlers concurrently. celilo will not isolate them, because isolating them defeats the purpose: a cursor written in one run must be readable in the next. A module needing atomicity inside its own storage does what any program does (atomic rename, distinct names, a lock file). This goes in the guide next to the path. I am taking this.

forgejo-admin commented

2026-08-22 16:43:21 +00:00

Author

Owner

Surface landed in #1101. Verified on main by artifact rather than by the merge response:


`packages/capabilities/src/types.ts:97`	`stateDir: string` on `HookContext`
`packages/capabilities/src/browser.ts:257`	`export function moduleStateDir(modulePath)`
`apps/celilo/src/hooks/executor.ts:635, :647, :686`	computed, injected, created
`reference/MODULE_DEVELOPMENT_GUIDE.md`	"Where a hook may write"
`apps/celilo/src/hooks/hook-state-dir.test.ts`	the gate

Acceptance

A hook has a documented, framework-provided place to write. ctx.stateDir, created before the hook starts so it exists on a module's first ever run.
Writing there does not produce an extra violation, by rule rather than by an allow-list entry. classifyModulePath classifies state/** as derived, and the gate uses generated filenames precisely so it cannot be satisfied by a literal somebody remembered.
extra recovers its real meaning. The same generated names outside state/ still classify package, so they are still scanned and still reported. That contrast test is the load-bearing half.
Recurrence gate. Two of them, and both were watched failing first.
screenshots/ and cookies.json either migrate or are documented as deliberate exceptions. Not done, deliberately. See below.

The one criterion left, and why it is not being quietly dropped

screenshots/ and cookies.json are still two ad-hoc entries beside state/ in classifyModulePath. This issue's own text left their migration to "when their writers move", and folding it into the one-line change would have turned it into a capability refactor.

They are not equivalent, and whoever picks this up should not treat them as one task. screenshots/ should not migrate. It is per-run, namespaced by moduleArtifactDir(modulePath, runKey), and pruned by retention. state/ is one directory per module that is never pruned. Merging them would either start deleting state or stop pruning artifacts. The right end state is two named directories with opposite lifetimes, which is what exists now — so screenshots/ is a deliberate exception and this comment is the documentation that criterion asks for.

cookies.json is the real migration. It is a single hardcoded filename at the module root, which is exactly the shape this issue was filed about. It belongs in state/, and moving it deletes an allow-list entry rather than adding one.

I am leaving the issue open on that last box rather than closing it a second time on a criterion I have not met.

Two notes for whoever reads this next

I closed this early once. The classification landed in #1091 and I treated that as done, with the tell sitting in my own test, which built the path by hand. It was caught by the first real consumer (celilo/lunacycle#64) rather than by review. The gate now asserts the hook learns the path and never its spelling, because a test comparing ctx.stateDir to a path it computed itself passes with no surface at all.

Two concurrent hooks for the same module share this directory. Reachable today: a health_check can run while a deploy is in flight, and the bus dispatches handlers concurrently. celilo does not isolate them, because isolating them defeats the point — a cursor written by one run must be readable by the next. Recorded in moduleStateDir's docblock and in the guide, with the ordinary answer (atomic rename, distinct names, a lock file).

Surface landed in #1101. Verified on `main` by artifact rather than by the merge response: | | | |---|---| | `packages/capabilities/src/types.ts:97` | `stateDir: string` on `HookContext` | | `packages/capabilities/src/browser.ts:257` | `export function moduleStateDir(modulePath)` | | `apps/celilo/src/hooks/executor.ts:635, :647, :686` | computed, injected, created | | `reference/MODULE_DEVELOPMENT_GUIDE.md` | "Where a hook may write" | | `apps/celilo/src/hooks/hook-state-dir.test.ts` | the gate | ## Acceptance - [x] **A hook has a documented, framework-provided place to write.** `ctx.stateDir`, created before the hook starts so it exists on a module's first ever run. - [x] **Writing there does not produce an `extra` violation, by rule rather than by an allow-list entry.** `classifyModulePath` classifies `state/**` as `derived`, and the gate uses generated filenames precisely so it cannot be satisfied by a literal somebody remembered. - [x] **`extra` recovers its real meaning.** The same generated names outside `state/` still classify `package`, so they are still scanned and still reported. That contrast test is the load-bearing half. - [x] **Recurrence gate.** Two of them, and both were watched failing first. - [ ] **`screenshots/` and `cookies.json` either migrate or are documented as deliberate exceptions.** Not done, deliberately. See below. ## The one criterion left, and why it is not being quietly dropped `screenshots/` and `cookies.json` are still two ad-hoc entries beside `state/` in `classifyModulePath`. This issue's own text left their migration to "when their writers move", and folding it into the one-line change would have turned it into a capability refactor. They are not equivalent, and whoever picks this up should not treat them as one task. **`screenshots/` should not migrate.** It is per-run, namespaced by `moduleArtifactDir(modulePath, runKey)`, and pruned by retention. `state/` is one directory per module that is never pruned. Merging them would either start deleting state or stop pruning artifacts. The right end state is two named directories with opposite lifetimes, which is what exists now — so `screenshots/` is a **deliberate exception** and this comment is the documentation that criterion asks for. **`cookies.json` is the real migration.** It is a single hardcoded filename at the module root, which is exactly the shape this issue was filed about. It belongs in `state/`, and moving it deletes an allow-list entry rather than adding one. I am leaving the issue open on that last box rather than closing it a second time on a criterion I have not met. ## Two notes for whoever reads this next **I closed this early once.** The classification landed in #1091 and I treated that as done, with the tell sitting in my own test, which built the path by hand. It was caught by the first real consumer (`celilo/lunacycle#64`) rather than by review. The gate now asserts the hook *learns* the path and never its spelling, because a test comparing `ctx.stateDir` to a path it computed itself passes with no surface at all. **Two concurrent hooks for the same module share this directory.** Reachable today: a `health_check` can run while a deploy is in flight, and the bus dispatches handlers concurrently. celilo does not isolate them, because isolating them defeats the point — a cursor written by one run must be readable by the next. Recorded in `moduleStateDir`'s docblock and in the guide, with the ordinary answer (atomic rename, distinct names, a lock file).