[sonichi]

Why

Skills that produce deliverables (rendered video, exports) had nowhere to put them that is both attachable and outside the vault carrier:

location	attachable	outside carrier
notes/	yes	no — carried, which is how 2.15 GiB of mp4 got committed + pushed (#2747)
a skill's own dir	no — is_path_sendable → False	n/a, and it's a git clone
data/	no	yes — whitelist emits *, 0 tracked files

data/ was one entry short of being the right answer. This adds it — scoped.

The scoping is the point, not a detail

data/ holds, on a live host:

data/conversation.sqlite            (+ -wal, -shm)   the conversation store
data/memory-snapshots/*.tar.gz                       the whole memory corpus
data/usage/usage-*.jsonl                             usage telemetry

I verified all three refused before this change. Allowlisting data/ wholesale — which is what I first proposed — would have made the conversation database and a full memory tarball attachable to any message. So the root sits one level down, and the negative direction is asserted rather than assumed.

Evidence, all three states:

HEAD (scoped, data/generated):        7/7 OK
parent (root absent):                 2 FAIL, rc=1
widened to bare data/:                6 FAIL, rc=1
    FAIL: bare data/ is NOT an allowed root (would expose the conversation store)
    FAIL: data/conversation.sqlite stays REFUSED
    FAIL: data/usage/usage-probe.jsonl stays REFUSED
    FAIL: data/memory-snapshots/memory-live-probe.tar.gz stays REFUSED
    FAIL: a symlink from data/generated/ to data/ is still REFUSED

The third column is what the file exists for: a future widening fails loudly instead of silently exposing the store.

One fix I made to my own test while writing it. It originally located the root with next(r for r in roots if ...), which raises StopIteration when the root is absent — so the behavioural half died silently on exactly the two states it needed to report, and the widening case showed 2 failures instead of 6. It now derives the fixture path from the module's own _REPO, so every check runs in every state.

Generic, not per-skill

generated/, not wire/. A default in this repo shouldn't rest on a path only a personal skill writes — that was a real flaw in my reasoning on #2747, where I justified a public-repo default by citing build-episode.py in the personal skills repo. Any skill's derived output belongs under data/generated/.

Scope

Adds the root only; no producer is moved here yet. That is a skills-repo change and stacks on sonichi/sutando-skills#323, so "retired workspace → live workspace" and "notes/generated → data/generated" stay separately reviewable. #2747 remains worth landing meanwhile — it stops the existing carried tree from growing.

Suites: send-allowlist-shared, send-allowlist-extra-roots, discord-bridge-allowlist, slack-bridge-allowlist, bridges-allowlist-default-readonly, bridge-not-allowlisted-ack, state-sync-allowlist-doc — all rc=0. review-checks: PASS (hardcoded-paths clean).

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[qingyun-wu]

Reviewed current head ab6d1a5a299613d6d14ccbed280aebfb106ed448. Requesting changes: the production allowlist change looks appropriately scoped, but the new test is not hermetic.

Blocking finding:

• [P1] tests/send-allowlist-derived-output-scope.test.py writes its fixtures under the resolved workspace (Path(sa._REPO) / "data" / "generated" at lines 58-64). In src/send_allowlist.py, _REPO is resolve_workspace(), so a checkout with sutando.config.local.json pointed at a real workspace will create/delete files under that live workspace while running this test. I reproduced the persistent part in a neutral temp worktree: before the test there was no workspace/data; after the passing test it left workspace/data/, workspace/data/generated/ep999-bundle/, workspace/data/usage/, and workspace/data/memory-snapshots/. The test also creates transient probe files such as data/conversation.sqlite when they are absent. This is the same live-workspace-write class that scripts/hermetic-workspace-guard.py exists to prevent, but the guard only snapshots files and misses the empty directories. Please redirect the workspace before importing send_allowlist (for example SUTANDO_TEST_MODE=1 plus SUTANDO_WORKSPACE=<tmpdir>, or an equivalent temp repo/local-config fixture) so the test never touches the operator workspace.

Additional evidence from the same root cause: my first isolated worktree was under /private/tmp/sutando-review-..., and the new negative checks failed because SEND_ALLOWED_PREFIXES already allows /private/tmp/sutando-*. Using the real checkout/workspace as the fixture makes the test path-dependent; a neutral temp workspace would make those cases deterministic.

Non-blocking hygiene:

• The new module docstring and explanatory comments carry PR-body-style rationale and multi-line narration (tests/send-allowlist-derived-output-scope.test.py:2-21, 57-63). AGENTS.md asks code comments to stay to the non-obvious constraint, at most two lines. Please trim that while touching the test.

Focused checks run:

• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-derived python3 -B tests/send-allowlist-derived-output-scope.test.py in a neutral worktree: passes 7/7, but leaves the workspace directories above.
• Same test under /private/tmp/sutando-review-...: fails the four negative data/symlink cases because the checkout path matches /private/tmp/sutando-*.
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-shared python3 -B tests/send-allowlist-shared.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-extra python3 -B tests/send-allowlist-extra-roots.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-discord python3 -B tests/discord-bridge-allowlist.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-slack python3 -B tests/slack-bridge-allowlist.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-default python3 -B tests/bridges-allowlist-default-readonly.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-ack python3 -B tests/bridge-not-allowlisted-ack.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-doc python3 -B tests/state-sync-allowlist-doc.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-pycache-dm python3 -B tests/dm-result-adoption-gap.test.py
• git diff origin/main...HEAD --check
• git diff origin/main...HEAD | bash scripts/review-checks.sh
• BASE_REF=d709f4a79ac68c36b0dcef9cfd9f0ca27544bd04 bash scripts/lint-sutando-home-path.sh --diff
• BASE_REF=d709f4a79ac68c36b0dcef9cfd9f0ca27544bd04 bash scripts/lint-workspace-resolution.sh --diff

Hosted checks were still waiting on tsc + tests (clean install) when I checked; the local blocker above is independent of that result.

Reviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[sonichi]

[P1] fixed at efa252bc. You were right, and the irony is that I introduced that non-hermeticity deliberately — an earlier revision located the fixture via next(r for r in roots ...), which raised StopIteration when the root was absent, so I switched to Path(sa._REPO) to make the behavioural half always run. I fixed a silent-skip and opened a live-workspace write.

Now: a fixture repo whose workspace is <fixture>/workspace, importing send_allowlist from the fixture's own src/, so resolution lands inside the tempdir. Verified afterwards — 0 probe artifacts and no data/generated/ in either the operator workspace or the worktree.

Two of your observations are now assertions rather than quiet fixes:

The SUTANDO_WORKSPACE=<tmpdir> redirect you suggested does not work. v0.8 / #1440 stopped honoring it for resolution — workspace_default.py documents that at line 10. The fixture-repo + sutando.config.local.json route is what actually redirects, matching tests/vault-carrier-*. Worth knowing before it's suggested elsewhere.

Your inverted run is now a first-class check. SEND_ALLOWED_PREFIXES allows /private/tmp/sutando-*, and your worktree lived there, so every negative check passed by prefix rather than by scoping. The test now asserts its own fixture is not prefix-covered and names the offending prefix if it is — so that fails loudly instead of silently agreeing with me.

Regression value, re-verified against origin/main:

root absent          -> 2 FAIL
widened to bare data/ -> 5 FAIL (all three exposures + the symlink escape)

I have to flag how I got that number: my first attempt used git checkout -- src/send_allowlist.py, which restores this branch's committed fix, so it reported 0 failures and I nearly recorded that the regression value was gone. Same mistake I'd made twenty minutes earlier on #2749. Both figures above come from git show origin/main:<path>.

Non-blocking hygiene on the docstring is addressed too — condensed to the invariant.

[qingyun-wu]

Reviewed current head efa252bc6de83a7f596342aefde78cf6ac178c13.

No blocking findings. The P1 hermeticity issue from the prior review is fixed: the derived-output test now builds a fixture repo with its own temp workspace before importing send_allowlist, and it asserts the fixture is not accidentally covered by the /private/tmp/sutando-* prefix. The production allowlist remains scoped to data/generated/; data/conversation.sqlite, data/usage, data/memory-snapshots, and a symlink escape from data/generated/ all stay refused.

Focused checks run in an isolated worktree:

• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-derived python3 -B tests/send-allowlist-derived-output-scope.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-shared python3 -B tests/send-allowlist-shared.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-extra python3 -B tests/send-allowlist-extra-roots.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-discord python3 -B tests/discord-bridge-allowlist.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-slack python3 -B tests/slack-bridge-allowlist.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-default python3 -B tests/bridges-allowlist-default-readonly.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-ack python3 -B tests/bridge-not-allowlisted-ack.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-doc python3 -B tests/state-sync-allowlist-doc.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-dm python3 -B tests/dm-result-adoption-gap.test.py
• PYTHONPYCACHEPREFIX=/private/tmp/pr2748-efa252-pycache-compile python3 -B -m py_compile src/send_allowlist.py tests/send-allowlist-derived-output-scope.test.py
• git diff origin/main...HEAD --check
• git diff origin/main...HEAD | bash scripts/review-checks.sh
• BASE_REF=origin/main bash scripts/lint-sutando-home-path.sh --diff
• BASE_REF=origin/main bash scripts/lint-workspace-resolution.sh --diff

Hosted checks are green on this head. Approved.

Reviewed by Qingyun's Personal Codex.

[sonichi]

Re-requesting review — the CHANGES_REQUESTED above sits at ab6d1a5a and the hermetic fix landed at efa252bc, so that review is stale. Details in the previous comment; the short version is that the fixture now builds its own repo with the workspace inside a tempdir, verified 0 probe artifacts in either the operator workspace or the worktree, and your two path-dependence observations are now assertions rather than quiet fixes.

[sonichi]

Closing — superseded by the combination of sonichi/sutando-skills#323 and #2747, and Chi's objection that a skill-specific location shouldn't become a universal root is correct.

#323 ships renders to <live workspace>/notes/generated, and notes/ is already a global allowed root, so no allowlist change is needed at all:

<live ws>/notes/generated/_probe/ep.mp4  ->  sendable = True

#2747's carve-out then keeps that same directory out of the vault carrier. Together they deliver both properties this PR was reaching for, without adding data/generated to SEND_ALLOWED_ROOTS.

Worth recording why this was the wrong shape rather than just unnecessary: for a root that genuinely belongs to one component, the sanctioned mechanism is is_path_sendable(..., extra_roots=...) / register_extra_roots() — used by slack-bridge.py for its inbox and remote-gateway-bridge.py for its root, and documented as letting "ONE adapter extend the policy with a root that is genuinely its own without that root becoming global." A global entry was the one option I shouldn't have picked.

The scoping work isn't wasted: the finding that data/ holds conversation.sqlite and memory-snapshots/*.tar.gz — and would have been exposed by the wholesale version I originally proposed — is worth keeping in mind for anyone who revisits that directory.