From b0579d571a9184d728848600cf6679d6b286c4e6 Mon Sep 17 00:00:00 2001 From: mikehasa Date: Thu, 30 Jul 2026 16:51:43 +0800 Subject: [PATCH] fix(docs): correct security claims in the new activation/capabilities prose Follow-up to #27 and #28. Those PRs are sound refactors, but a few of the docstrings they added assert safety properties the code does not provide, and one of those claims justified dropping a real defense. - activation: restore the per-acquisition chmod on the lock file. `os.open` applies its mode ONLY on creation, so a lock left group/world-readable by an older run stayed that way; `main` re-tightened it on every `_locked()` entry. Adds a regression test that fails without the chmod. - activation: `_write_json_atomically` documented the durability order backwards. The parent-directory fsync happens AFTER the rename -- that is what makes the rename durable -- and the helper is advertised for reuse. - activation: `_try_chmod` claimed failing silently is always safe. It is not at the spawn log, which `Path.open("ab")` creates at 0o666 & ~umask, where this chmod is the only thing keeping child stdout/stderr private. - capabilities: the validator checks the SHAPE of an evidence claim, not its truth -- it never opens the referenced document. Say so instead of promising the matrix "cannot overclaim". - capabilities: `experimental` was defined as "only tested with synthetic data", but codex.model_attribution and cursor.mcp_semantics carry no evidence at all. Widen the definition to match the data. --- src/agentacct/activation.py | 29 ++++++++++++++++++++--------- src/agentacct/agent_capabilities.py | 11 ++++++++--- tests/test_activation.py | 22 ++++++++++++++++++++++ 3 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/agentacct/activation.py b/src/agentacct/activation.py index 39ec381..d70ee87 100644 --- a/src/agentacct/activation.py +++ b/src/agentacct/activation.py @@ -152,10 +152,16 @@ def _try_chmod(path: Path, mode: int) -> None: """Best-effort ``chmod``: apply ``mode`` and ignore ``OSError``. This is a generic permission setter -- the *intent* (owner-only) lives in - the ``OWNER_ONLY_*`` constants each caller passes. It is allowed to fail - silently because every state file and directory is already created - owner-only, so this only closes the umask gap on paths that pre-existed; the - strict create-time modes keep the data private regardless. + the ``OWNER_ONLY_*`` constants each caller passes. Ignoring ``OSError`` + keeps a chmod that cannot succeed (exotic filesystem, foreign owner) from + breaking an otherwise working install. + + That tolerance is NOT uniformly harmless, so do not "simplify" a call site + away on the assumption that it is: most callers create their file or + directory with the mode already applied and use this only to re-tighten a + pre-existing path, but the spawn log is opened with ``Path.open("ab")`` -- + i.e. ``0o666 & ~umask`` -- so there this chmod is the ONLY thing keeping + child stdout/stderr (store paths, argv, tracebacks) private. """ try: path.chmod(mode) @@ -181,10 +187,11 @@ def _ensure_owner_only_dir(directory: Path) -> None: def _write_json_atomically(path: Path, value: Mapping[str, Any]) -> None: """Write JSON to ``path`` so a crash mid-write can never leave a partial file. - Writes a fresh temp file, fsyncs the bytes and the parent directory (so the - rename is durable on disk), then atomically moves it into place. Readers - therefore always see either the complete previous value or the complete - new value -- never a half-written mixture. + Writes a fresh temp file and fsyncs its bytes, atomically moves it into + place, and only THEN fsyncs the parent directory -- that last step is what + makes the rename itself durable, so the order matters. Readers therefore + always see either the complete previous value or the complete new value -- + never a half-written mixture. """ _ensure_owner_only_dir(path.parent) temporary = path.parent / f".{path.name}.tmp-{os.getpid()}-{uuid.uuid4().hex}" @@ -222,8 +229,12 @@ def _flock_exclusive(lock_path: Path) -> Generator[None, None, None]: # A pure lock handle: nothing is read or written, so there is no append or # binary mode to reason about. O_CLOEXEC stops the fd leaking into child # processes we spawn (Python's open() sets this by default; os.open does - # not). Mode OWNER_ONLY_FILE is applied at creation, so no separate chmod is needed. + # not). The mode argument applies ONLY when os.open creates the file, so a + # lock left group/world-readable by an older run would keep those bits: the + # chmod below re-tightens it on every acquisition, exactly as the + # pre-extraction `_locked()` bodies did. fd = os.open(lock_path, os.O_RDWR | os.O_CREAT | os.O_CLOEXEC, OWNER_ONLY_FILE) + _try_chmod(lock_path, OWNER_ONLY_FILE) try: fcntl.flock(fd, fcntl.LOCK_EX) yield diff --git a/src/agentacct/agent_capabilities.py b/src/agentacct/agent_capabilities.py index 5dc77b1..d020b01 100644 --- a/src/agentacct/agent_capabilities.py +++ b/src/agentacct/agent_capabilities.py @@ -42,7 +42,8 @@ State -- how real a lane is: unavailable not implemented - experimental implemented, but only tested with synthetic data + experimental implemented, but unproven: evidence is synthetic, or + (see codex.model_attribution, cursor.mcp_semantics) absent verified_partial works, with real evidence but narrow scope verified works, with real evidence @@ -56,8 +57,12 @@ none | manual_manifest | manual_profile | opt_in_project | one_command_project The honesty rule: a "verified*" state requires real, dated evidence, and a -synthetic fixture can never prove a verified state. The validator enforces -this, so the published matrix cannot overclaim. +synthetic fixture can never prove a verified state. The validator enforces the +SHAPE of that claim -- a parseable ISO date plus non-empty, non-traversing +relative refs -- but it never opens the referenced document, so it cannot tell +a real citation from a plausible-looking one. Whether the evidence behind a +"verified" row actually says what the row claims stays a maintainer's +responsibility, not a guarantee this module can make. """ from __future__ import annotations diff --git a/tests/test_activation.py b/tests/test_activation.py index 0fc0d4d..81024bc 100644 --- a/tests/test_activation.py +++ b/tests/test_activation.py @@ -259,6 +259,28 @@ def test_install_record_is_owner_only_deduped_and_idempotent(tmp_path) -> None: assert repeated["clients"] == ["claude-code", "codex"] # unchanged (codex already in) +def test_acquiring_the_lock_retightens_a_loosened_lock_file(tmp_path) -> None: + """A pre-existing world-readable lock file is re-tightened on acquisition. + + Significance: ``os.open``'s mode argument applies ONLY when it creates the + file, so a lock left group/world-readable by an older run (or a stray + ``touch`` under a permissive umask) would keep those bits forever. Every + lock acquisition therefore re-chmods, which is the self-heal the original + ``_locked()`` bodies performed inline. Without it this test sees 0o644. + """ + store = ActivationStateStore(tmp_path / "state") + store.mark_configured(project_dir=tmp_path, clients=["codex"]) + assert store.lock_path.exists() + + # Simulate the damage: an older run left the lock world-readable. + store.lock_path.chmod(0o644) + + # Any operation that takes the lock must repair it. + store.mark_configured(project_dir=tmp_path, clients=["claude-code"]) + + assert stat.S_IMODE(store.lock_path.stat().st_mode) == 0o600 + + def test_mark_configured_requires_at_least_one_client(tmp_path) -> None: """An empty client list is rejected -- the boundary must name a real client.