Fix what review found in the skill: five real defects
Adversarial review of #29 found bugs I confirmed by reproducing each one. **Every hand-written error message was unreachable.** `tok=$(curl ...)` is a plain assignment, so `set -e` acts on the command substitution before the following `|| die` can run. A wrong password produced exit 22 and no output at all — the most likely way this gets used wrongly, and the least explained. All four captures now go through a `run` helper that takes a *description* rather than echoing the command, because one of them carries the account password in `-u`. **`up` was not idempotent, and the second run destroyed DNS.** The resolv.conf backup was copied unconditionally, so `up --full` twice overwrote the good backup with PIA's own resolvers; the later `down` then "restored" those and left the container with no working DNS and no way back. `up` now runs `down` first. Verified: two `up --full` runs, then `down`, and the backup still holds the original 192.168.65.7. **An empty gateway produced total connectivity loss, reported as healthy.** `$gw` was never validated and `add_route` swallowed every failure to /dev/null. The two half-routes need no gateway and would succeed, so the tunnel captured everything while the exclusions keeping DNS and the Docker host reachable silently did not exist — and `status` still printed "full tunnel". Routes are now fatal on failure, and a via-less default (`$3` is the literal "eth0") is rejected. **The PIA session token was in the process arguments** — confirmed in `ps` and /proc/*/cmdline, a ~24h bearer credential for the account readable by anything in the container. It now goes to curl on stdin as a config. Verified: 60 polls across a full `up`, zero sightings. **The preflight diagnosed the wrong kernel module.** It checked /dev/net/tun and blamed the tun module, but kernel WireGuard is a netlink interface and does not use it — verified by creating one with NET_ADMIN and no tun device. The check is dropped (the container could not have started without the device anyway) and `ip link add` now reports the real dependency. Also: a full tunnel with no DNS servers from PIA used to warn and carry on, which is a tunnel leaking every lookup while reporting itself healthy — now fatal. `down` validates the backup before restoring it, so a truncated one cannot leave the container with no resolver at all. `wg.priv` is shredded on teardown and created under umask 077, because /run rides `docker commit` into the snapshot image. A mistyped `up --ful` is rejected instead of silently giving a test route. entrypoint: `install_feature_skill` gets `local`, a blank-name guard (the disabled branch would otherwise `rm -rf` the whole skills directory under a persisted volume), `-e`/`-L` so a leftover *file* at the destination is cleaned up, and a chown of the parent so `claude` can still add skills of their own when Mission Control is off. When the base image predates the skill it now says so instead of returning silently — and `/opt/triple-c-skills` joins FEATURE_PROBES so the migration pre-flight reports it. Docs corrected to match: neither half reaches an existing project without a migration. `vpn_env_var` extracted and tested, pinning the property the whole removal path rests on — that the variable is emitted as 0 rather than omitted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -841,6 +841,23 @@ type VpnHostConfigParts = (
|
||||
/// host-kernel module auto-loading. It is also enough to flush netfilter rules
|
||||
/// inside the container, so pair it with `sandbox_mode_enabled` advisedly.
|
||||
/// Hence opt-in, per project, rather than on for everyone.
|
||||
/// The env var `entrypoint.sh` installs and removes the `pia-vpn` skill from.
|
||||
///
|
||||
/// **Emitted either way, never omitted.** `~/.claude` is a persisted volume, so
|
||||
/// turning the toggle off has to actively tell entrypoint to remove a skill an
|
||||
/// earlier run left there, and an absent variable cannot say that. It is also
|
||||
/// what stops a `=1` baked into a snapshot by `docker commit` from outliving
|
||||
/// the setting — the explicit `=0` overwrites it.
|
||||
///
|
||||
/// Extracted for the same reason as [`vpn_host_config`]: the emitting code sits
|
||||
/// in a very long function where a dropped or inverted value is invisible, and
|
||||
/// `MISSION_CONTROL_ENABLED` twenty lines above shows the failure this avoids —
|
||||
/// it is pushed only when true, so a snapshot's baked `=1` survives the toggle
|
||||
/// going off.
|
||||
fn vpn_env_var(enabled: bool) -> String {
|
||||
format!("VPN_SUPPORT_ENABLED={}", u8::from(enabled))
|
||||
}
|
||||
|
||||
fn vpn_host_config(enabled: bool) -> VpnHostConfigParts {
|
||||
if !enabled {
|
||||
return (None, None, None);
|
||||
@@ -1276,14 +1293,7 @@ pub async fn create_container(
|
||||
env_vars.push("MISSION_CONTROL_ENABLED=1".to_string());
|
||||
}
|
||||
|
||||
// Drives the pia-vpn skill install in entrypoint.sh. Sent as 0 rather than
|
||||
// omitted when off, because ~/.claude is a persisted volume: entrypoint has
|
||||
// to be told to *remove* a skill left there by an earlier run with the
|
||||
// toggle on, and an absent variable cannot say that.
|
||||
env_vars.push(format!(
|
||||
"VPN_SUPPORT_ENABLED={}",
|
||||
u8::from(project.vpn_support_enabled)
|
||||
));
|
||||
env_vars.push(vpn_env_var(project.vpn_support_enabled));
|
||||
|
||||
// Permission mode — read by triple-c-task-runner for scheduled (headless)
|
||||
// Claude Code runs. Interactive terminals get the flags directly instead.
|
||||
@@ -2799,6 +2809,17 @@ mod tests {
|
||||
assert_eq!(cap_add.unwrap(), vec!["NET_ADMIN"]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_vpn_skill_flag_is_emitted_either_way_never_omitted() {
|
||||
// The whole removal path depends on this. If `false` ever became "emit
|
||||
// nothing", a project that had the toggle on would keep the skill
|
||||
// forever: the container recreates from a snapshot whose baked
|
||||
// VPN_SUPPORT_ENABLED=1 would then go unchallenged, and entrypoint
|
||||
// would reinstall a skill for a capability the container no longer has.
|
||||
assert_eq!(vpn_env_var(true), "VPN_SUPPORT_ENABLED=1");
|
||||
assert_eq!(vpn_env_var(false), "VPN_SUPPORT_ENABLED=0");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_vpn_skill_flag_is_reserved_from_custom_env() {
|
||||
// entrypoint.sh installs and removes the pia-vpn skill from this
|
||||
|
||||
@@ -136,6 +136,7 @@ pub const FEATURE_PROBES: &[(&str, &str)] = &[
|
||||
("/usr/local/bin/triple-c-sso-refresh", "AWS SSO auto-refresh"),
|
||||
("/opt/mission-control", "Mission Control (Flight Control)"),
|
||||
("/usr/bin/wg", "VPN support (WireGuard tools)"),
|
||||
("/opt/triple-c-skills", "Feature skills (PIA VPN)"),
|
||||
];
|
||||
|
||||
/// Headroom demanded on Docker's storage backend on top of the measured
|
||||
|
||||
Reference in New Issue
Block a user