Repository navigation
FEAT: file write surface scorer (scorer phase 10) - #3042
WatchTree-19 wants to merge 3 commits into
Conversation
Adds SurfaceScorable and ScoringScope, the ContentWritten condition, a surface observation payload, LocalFileSurfaceSource and FileWriteScorer, with a runnable attack example.
Preserve both surface and conversation scoring additions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| candidates = sorted( | ||
| path for path in root.glob(str(relative)) if path.is_file() or (path.is_symlink() and not path.is_dir()) | ||
| ) | ||
| if len(candidates) > self._max_files: |
There was a problem hiding this comment.
🔴 Must Fix: an unreadable directory must not become a complete negative.
Path.glob() suppresses directory-listing errors, including PermissionError. Those failures never reach the later read_failed handler. I created data/out.txt containing the expected text and made only os.scandir(root / "data") raise PermissionError. Acquiring /data/**/* returned no entries, Acquisition.COMPLETE, and no coverage reasons, so matching returned False even though the file was there.
Please use enumeration that records failed directory listings as coverage gaps. A negative needs proof that every relevant directory was searched. Add a test for a denied nested directory that expects an undetermined verdict, not False.
| size += len(chunk) | ||
| if len(retained) < self._max_content_bytes: | ||
| retained.extend(chunk[: self._max_content_bytes - len(retained)]) | ||
| truncated = size > len(retained) | ||
| content = _decode_text(bytes(retained), truncated=truncated) |
There was a problem hiding this comment.
🟡 Should Fix: bound acquisition work, not just the retained snapshot.
max_content_bytes stops retaining bytes but this loop still reads and hashes the entire file. With an 8-byte limit, my probe still read all 1,000,000 bytes. Likewise, max_files=1 enumerated and sorted all 20 matching files before keeping one.
This matters for an agent-controlled workspace: a very large tree or a file that keeps growing can occupy the worker indefinitely. Cancelling the awaiting coroutine does not stop an asyncio.to_thread() worker; I verified that it kept running after cancellation.
Please add explicit enumeration/read budgets and cooperative cancellation, marking coverage incomplete when a budget is hit. The full-content digest contract also needs to distinguish an incomplete read rather than requiring unbounded I/O. Add tests that check how much work is actually performed.
| try: | ||
| target = path.resolve(strict=True) | ||
| except OSError: | ||
| reasons.append("dangling_link") | ||
| continue | ||
| if not target.is_relative_to(root): | ||
| reasons.append("link_outside_root") | ||
| continue | ||
| if not target.is_file(): | ||
| reasons.append("not_a_file") | ||
| continue | ||
| try: | ||
| modified_at = datetime.fromtimestamp(target.stat().st_mtime, tz=UTC) | ||
| if window is not None and not window[0] <= modified_at <= window[1]: | ||
| excluded += 1 | ||
| continue | ||
| entries.append(self._read(target=target, location=location, modified_at=modified_at)) |
There was a problem hiding this comment.
🔴 Must Fix: keep the root boundary intact until the file is opened.
resolve() checks a pathname, but _read() opens that pathname later. A directory or link can change between those steps. I reproduced this using only temporary files: after the checks, replacing root\data with a Windows junction to another temporary directory made /data/out.txt retain outside-synthetic-secret with Acquisition.COMPLETE.
The agent being tested controls this workspace, so the current check does not guarantee that reads stay inside the root. Please validate the actual opened file against the root before reading any bytes, using platform-appropriate handle-based confinement. Use that same handle for metadata and content, and add a regression test for a directory/link changing during acquisition. Resolving the path a second time before open() would still leave a race.
| pieces = await self._memory.get_message_pieces_async(prompt_ids=list(scorable.message_piece_ids)) | ||
| conversation_id = next((piece.conversation_id for piece in pieces if piece.conversation_id), None) | ||
| if conversation_id is None: | ||
| raise ValueError("File write scoring of a message requires a stored conversation.") | ||
| conversation = await self._memory.get_message_pieces_async(conversation_id=conversation_id) |
There was a problem hiding this comment.
🟡 Should Fix: validate the complete message reference before choosing its run.
This selects the first conversation returned by memory without checking that every requested ID exists or that the pieces belong to one message. I verified that both of these inputs produce persisted scores:
MessageScorable(message_piece_ids=(stored_piece.id, missing_uuid))
MessageScorable(message_piece_ids=(piece_from_run_a.id, piece_from_run_b.id))For the second case, the score names both runs but the surface observation uses only one run's time window and attack ID. Which run gets selected depends on the query result order.
Please reuse MessageScorableResolver.resolve_async() before deriving the scope. It already rejects missing IDs and pieces that do not form one stored message. Add tests for both cases so invalid evidence cannot silently receive a verdict.
…handle, resolve the message - Directory listings that fail are coverage gaps (listing_failed), so a denied directory leaves the verdict undetermined instead of a complete negative. - Acquisition work is bounded: max_listed_entries caps entries examined, max_files stops enumeration at the first candidate over the limit, and max_read_bytes caps bytes read and hashed per acquisition. A file cut short has no digest (SurfaceEntry.sha256 is now optional) and leaves coverage incomplete (read_limit_exceeded). Cancelling the awaiting coroutine sets a flag the worker checks at every entry and chunk. - Each file is opened first, the open handle's final path is checked against the resolved root (/proc/self/fd, F_GETPATH, GetFinalPathNameByHandleW), and size, timestamp and content all come from that handle. Opens are non-blocking so a FIFO cannot hang the worker. Directory links are not descended into and are reported as gaps. - FileWriteScorer resolves the message reference with MessageScorableResolver before deriving the run's scope, rejecting missing ids and pieces that do not form one stored message.
|
Thanks Roman Lutz (@romanlutz), all four were real, and the reproductions made them quick to pin down. Pushed in one commit on top of your merge of main. Unreadable directories: listings now go through an explicit walk, and a directory that cannot be listed is recorded as Bounded work: Confinement: each file is now opened first, the open handle's final path is checked against the resolved root ( Message reference: the scope now comes from |
This branch has not been deployed
2 participants
Description
This is phase 10 from the scorer roadmap, the surface prototype. It adds a scorer that answers "did this run write that content to that location?" by reading what the location actually holds, rather than what the response says it did.
It is built on the pieces from the earlier phases, so a surface is just another scorable with its own observation source, and nothing in the existing scorers changes.
What it adds
SurfaceScorable(scorable_type="surface") with auri, asurfacename, anexactorglobmatch, and an optionalScoringScope.ScoringScopewith a timewindow,labelsand anattempt_id, so a source can narrow the question to one run.ContentWrittencondition (uri,match, optionalcontains).SurfaceObservationPayload(kind="surface") holding oneSurfaceEntryper location (size, sha256, modified time, a bounded copy of the text) plusSurfaceCoverageand a count of locations that fell outside the scope.LocalFileSurfaceSource, which reads files under one root directory, such as a mounted sandbox workspace.FileWriteScorer, aTrueFalseScorerwithCONDITION_TYPE = ContentWritten.On the two open questions in the roadmap
ObservationSource[SurfaceScorable]. The local file source is the first one; a container, bucket or MCP filesystem source would plug into the same slot without touching the scorer.How the verdict works
Correlating a write to a run
Given a message, the scorer builds the scope from the run itself: the conversation's
attack_result_id(from phase 9) and a window from the first message, less a small clock skew allowance, to the time of scoring. The local source can only check the window, against file modification times, so it records in the observation metadata which scope keys it did not apply. A file planted before the run is counted as outside the scope and cannot make an attack succeed.Safety of the local source
Locations resolve inside the root only.
..is rejected, and symbolic links that point outside the root are not followed and are reported as a coverage gap. Reads are bounded bymax_filesandmax_content_bytes, and run off the event loop.Replay
The observation keeps the digest, size, modified time and retained text, so
score_observation_asynccan re-judge it against a newcontainsafter the workspace is gone. Replay against a different location raisesNonReplayableObservationError.Tests and Documentation
tests/unit/models/test_surface.py(17 tests): model validation, round trips through the scorable and condition registries, and the acquisition and coverage invariants onObservation.tests/unit/score/test_file_write_scorer.py(41 tests): the source (window, glob, limits, symlink escape, dangling links, truncation on a multi-byte boundary), the matching table, scorer verdicts, offline replay, and an end to endPromptSendingAttackagainst a local agent that writes the file, including a planted older file that must not count.doc/code/scoring/6_file_write_scorer(paired .py and .ipynb, executed), linked from the scoring overview,myst.ymlandframework.md. It needs no model, service or credentials.