Act on the review of the decoupling branch #152
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: dejima/dejima#152
Reference in New Issue
Block a user
Summary generated by LLM agent
Acts on the review left at
~/shared/pr133-review.md. Items 1 and 3-9; item 2 is #133's own description and base, handled separately.1. The shipper dropped the tail of every session
The review is right and this is the one that mattered.
bin/dejimaSIGTERMs the shipper the moment the container exits, and it had no trap - so the in-memory batch and everything written since the last poll went with it. That is typically the agent's final answer.Worse, the comment at the kill site claimed the opposite ("the last lines a session produced are shipped rather than left in a batch this kill discards"). I asserted a property I had not tested; a comment that lies about a security-adjacent log is worse than no comment.
Fixed with a TERM trap that scans once more and flushes. Verified both ways: wrote a line, killed immediately, found it shipped - and against the pre-fix code, confirmed it is lost. Also exits when
bin/dejimawas SIGKILLed, rather than polling an ended session forever.3. Nothing said when the hooks were never registered
An operator who set
[core] harnesswithout re-running./install.shgot no briefing and no guards, on every launch, silently. Now a launch says so, in the same voice as the guard report -harness_guards_registeredis a plugin hook, since where a harness records its hooks is its own business, and a harness that cannot answer is assumed wired rather than refused.5. The section rename broke the property boundaries exist for
A state file written before the rename, compared after, sent the old
### statuslineentry into the tcb range: a CRITICAL for a file nothing touched. The old boundary is now read as the new one when the new one is absent, with a test that writes a pre-rename state file and asserts a clean compare.6. Wrapper name collision
identifierpermits any bare name, socommand = gosuwould have had the entrypoint exec the harness as root. The manifest is trusted, so this is not a hole - it turns a bad manifest into a clean refusal instead of undefined behaviour. Existing symlinks to the wrapper are still fine, for idempotence.4, 7, 8, 9
docs/security.mdnames the trade in host-side shipping:jqnow parses session-controlled bytes outside the container as the operator. That is the cost of a log a session cannot silence, and it should not read as a free win./etc/resolv.confand/etc/hostsjoinDEJIMA_CONTAINER_PATHS- the DNS pin routing the model API to the TLS terminator is as load-bearing as anything already there.command_pathis refused at launch, beside theno_confirm_argsrefusal.no_confirm_argsandsettings_argstake a newargstype.templatepromises substitution against the briefing whitelist and these never reach composition, so a{{...}}shipped literally.One test the fixes corrected
"a harness hosting every guard says nothing about guards" asserted on the bare word
guards, and the new not-registered warning contains it. That assertion was too broad - a different message about a different problem. It matches the guard report's own wording now.Testing
static.bats250,config.bats127,audit.bats158,providers.bats70,remote.bats104,project.bats134,proxy.bats30 - no failures.The conversation shipper dropped the tail of every session. bin/dejima SIGTERMs it the moment the container exits and it had no trap, so both the in-memory batch and everything written since the last poll went with it - typically the agent's final answer. The comment at the kill site claimed the opposite, which is worse than saying nothing. It traps TERM now, scans once more and flushes; confirmed by writing a line, killing immediately, and finding it shipped, against a control that loses it. It also exits if bin/dejima was SIGKILLed rather than polling an ended session forever. Nothing said when the hooks were never registered. install.sh registers them gated on [core] harness, so an operator who set that key without re-running it got no briefing and no guards, on every launch, silently. The launch says so now, in the same voice as the guard report. The audit's section rename broke the one property those boundaries exist for: a state file written before it, compared after, sent the old statusline entry into the tcb range and produced a CRITICAL for a file nothing touched. The old boundary is read as the new one when the new one is absent. The entrypoint refuses a wrapper name that already exists in /usr/local/bin. `identifier` permits any bare name, so `command = gosu` would have had the entrypoint exec the harness as root - the manifest is trusted, so this turns a bad one into a clean refusal. /etc/resolv.conf and /etc/hosts join the container paths a harness mount may not shadow: the DNS pin that routes the model API to the TLS terminator is as load-bearing as anything already listed. An empty command_path is refused at launch beside the no_confirm_args refusal, rather than as the wrapper's exit 127 inside a started container. no_confirm_args and settings_args take a new `args` type. `template` promises substitution against the briefing whitelist, and these never pass through composition, so a {{...}} in one shipped literally. docs/security.md names the trade in moving the transform host-side: jq now parses session-controlled bytes outside the container as the operator, which is the cost of a log a session cannot silence.P4.17, P4.15, P4.16 and P3.14 from the codebase audit. shellcheck now runs over all 29 shell scripts in the repo as its own CI job, at -S warning. test/shellcheck.sh holds the file list, discovered by shebang rather than enumerated, so a script added under bin/, hooks/, docker/ or a forge's bin/ is covered the day it lands; the job and the static test that pins its coverage call the same script. It found a real bug on the first run. _provider_keys_cached did local axis="$1" name="$2" cache_key="$axis:$name" and bash does not apply the earlier assignments before evaluating the later one in a single `local`, so cache_key was built from whatever axis and name meant in the CALLER's scope. The cache was keyed on ":" for every provider, and the only reason nothing broke is that both call sites wrap it in $( ) - so the cache was populated in a throwaway subshell, read back never, and its comment described a win that could not happen. It is removed rather than fixed: a per-command-substitution cache with one lookup per subshell is dead code, and dead code with a latent collision in it is worse than none. Six `export X="$(cmd)"` in bin/dejima are split into an assignment and an export. export always succeeds, so the combined form hides a failing substitution from set -e; split, a config_get that fails stops the launch. The rest of what shellcheck flagged is intentional, and says so at the site: one-row tables that read as loops, literal tildes in prose, a trap body baked in on purpose, and mkdir -p -m whose leaf-only mode is the reason for the verify-and-chmod block beneath it. Two classes are disabled repo-wide in .shellcheckrc, both about variables crossing a boundary shellcheck cannot see: a library defining what its consumers read, and status=$? inside a single-quoted trap. Verified clean under 0.11.0 and under the 0.9.0 CI will install. test/conversation.bats replaces grep-based assertions about bin/dejima-conversation-ship with eight behavioural ones: the shipper runs against a real transcript and a real HTTP endpoint standing in for Loki, and what arrives is what is asserted on. Mutation-tested both ways - removing the TERM trap fails the two loss-path tests, and a final_flush that flushes the batch without rescanning fails only the tail test, which is the distinction a grep cannot make. The superseded grep test is removed. The web/index.html staleness test compared the manifest against the tree and nothing against the page, so a commit staging web/src and web/sources.sha256 while leaving index.html behind passed and shipped the stale page - the exact failure its own comment claimed to prevent. build.mjs already stamps the page with sha256sum of the manifest it wrote; the test now checks that too. Verified by reproducing the stale-page commit, which fails, and the correct one, which passes. build.mjs pointed at test/static.bats for this; the test is in test/remote.bats. One static test hardcoded "exactly 2 checkouts pin the head sha", which the new job broke. It asserts the property now - every checkout is pinned except the two runner-env jobs - since a count breaks when a job is added, which is when nobody is thinking about which ref it reads. All eight host-side suites pass, and shellcheck is clean.P2.8 and the P5 items from the codebase audit. docs/development.md's decoupling inventory listed "Settings layering" and "Staged config" as resolved while bin/dejima still pre-created ~/.claude/settings.json and ~/.claude/CLAUDE.md by name, parsed settings.json with Claude-Code-specific reasoning, and generated the override layer as {sandbox: {enabled: false}} - Claude Code's settings schema, not a neutral one. The pre-creation was a duplication as well as a coupling, since harness_staged_config already declares exactly those two paths. All three are one plugin hook now, harness_prepare_config, which core calls with the path to write the override to. The plugin decides which config files must exist, what makes one unusable, and what the override says; core keeps what was always its own - that the override is a throwaway the session cannot reach, that it is mounted at a container-local path and never over the harness's own settings file (#47), and that a plugin refusing there stops the launch. A settings schema is a parser's grammar rather than a value, which is why this is a plugin hook and not a manifest key. statusline.sh is deliberately not among the files created if absent: it is staged executable and named by settings.json's statusLine.command, so an empty one would be a status line that runs and does nothing in place of the harness having none. The inventory now describes what is there, with a second row for the config preconditions. The credential proxy read a request body of any size into host RAM, from a client that is the sandboxed session. Every other session-controlled size in the repo is bounded, several with explicit reasoning about $TMPDIR being tmpfs. Capped at 32MiB and refused with a 413 rather than truncated, since half a request forwarded upstream is worse than a rejected one. The refusal raises its own exception type: _read_body also raises ValueError for a genuinely malformed request, and a test pins that such a fault still surfaces as a traceback - catching ValueError here would have swallowed it, which is the trade the suppression elsewhere in that file exists to avoid. dejima-epitaph was silent when no harness is selected, exiting 0 having saved nothing - indistinguishable from a session that never compacted. It says so now, in the same voice dejima-audit uses for the identical condition. dejima-attach's picker exited 1 with no message when there was no controlling terminal (ssh -T, or an SSH app that requests no pty), which reads as a broken front door. `read -r choice || exit 0`, matching what attach() already reasons about explicitly. Six new tests: the override disables the harness's own sandboxing, carries none of the operator's other settings, refuses an unparseable settings.json, creates both config files but not statusline.sh, leaves an existing file alone, and core names no harness config file of its own. Two more in proxy.bats for the body cap in both directions. A launch's 57 docker arguments are identical before and after. All nine host-side suites pass, and shellcheck is clean.