Repository navigation
FEAT: observation lifecycle for surface scoring (scorer phase 11) - #3156
WatchTree-19 wants to merge 2 commits into
Conversation
A snapshot shows that content is present; it cannot show that this attempt wrote it. Lifecycle support is optional and lets a source record a baseline when an attempt starts and finalize a later snapshot against it, so attempts can share a workspace without losing write attribution. - SurfaceBaseline records what the named locations held, with the same bounded evidence and coverage a snapshot keeps. A finalized SurfaceObservationPayload carries its baseline, so replay gives the same verdict. - SurfaceLifecycleSource and AttemptObserver are runtime protocols. LocalFileSurfaceSource implements start_observing_async and finalize_async on the same bounded, confined read as acquire_async. - An attack asks its objective and auxiliary scorers that observe attempts to start once the attempt ID is allocated, before anything is sent, and to stop when the attempt ends, whatever the outcome. - FileWriteScorer records a baseline per attempt and routed condition, and scoring finds the attempt from the conversation's attack result. A location counts only when the attempt changed it and added the content: unchanged is a proven negative, the same bytes written again cannot be credited, an append is credited only for a match that reaches into the new text, and a location the baseline might have missed stays undetermined. Without lifecycle support the scorer keeps the snapshot.
| prior = baseline.entry_for(entry.uri) | ||
| if prior is None: | ||
| changed: bool | None = True if baseline.coverage.complete else None | ||
| before: str | None = "" if baseline.coverage.complete else None | ||
| else: |
There was a problem hiding this comment.
🔴 Must Fix [P1]: A baseline does not identify which overlapping attempt wrote the file.
The new documentation says lifecycle support removes the requirement to run shared-root attempts one at a time. A before/after comparison cannot provide that guarantee: two attempts can record the same empty baseline, one writes marker, and the other writes nothing. Both then see a new matching file and receive SUCCESS.
I reproduced this with two concurrent PromptSendingAttack.execute_async calls sharing one FileWriteScorer. A barrier lets both sends start before the writer creates the file. The idle attempt's persisted score says "The attempt wrote ...", although it never wrote anything.
Please retain an explicit exclusive-access requirement for shared workspaces, or add attempt-specific write evidence before claiming attribution for overlapping attempts. The lifecycle alone can remove the need to clear old content, but it cannot remove workspace isolation or serialization. Add a concurrent writer/nonwriter regression and update the new documentation accordingly.
There was a problem hiding this comment.
Thanks Roman, agreed, a baseline can't separate overlapping attempts. Fixed in 3568a42: the docs keep the one-at-a-time requirement for a shared root (the lifecycle only removes the need to clear it), and when a FileWriteScorer sees another attempt observing the same location at the same time it leaves a change undetermined rather than crediting it. SurfaceBaseline.concurrent carries that into the stored observation so replay agrees. The new test is your barrier setup: writer and idle both come back undetermined, and a sequential control keeps SUCCESS then FAILURE.
| try: | ||
| for scorer, expectation in self._attempt_observers(context=context): | ||
| started.append(scorer) | ||
| await scorer.start_observing_async(attempt_id=attempt_id, expectation=expectation) |
There was a problem hiding this comment.
🔴 Must Fix [P2]: Persist failures while starting observation.
start_observing_async runs before Strategy.execute_with_context_async, which owns the ON_ERROR event and error-result persistence. If a lifecycle source raises here, the allocated attempt ID has no stored ERROR result and no confirmed result ID for the executor to return. The outer handler only checks an already-recorded persistence error; it does not record this failure.
I reproduced this by making LocalFileSurfaceSource.start_observing_async raise OSError("baseline acquisition failed"). The exception reaches the caller and the baseline is released, but get_attack_results_async(attack_result_ids=[context.attack_result_id]) returns no rows.
Please put observation startup inside the attack's error-recording boundary, or explicitly dispatch the same error handling for startup failures. Add a regression that checks the saved ERROR result and its allocated attempt ID, not only cleanup.
There was a problem hiding this comment.
Fixed in 3568a42. Observation now starts in _execution_context_async, after setup and inside the strategy's execution, so a failing start goes through ON_ERROR and the ERROR result is saved under the allocated ID. The regression raises OSError from the source's start and checks the saved row by that attempt ID.
| finally: | ||
| for scorer in started: | ||
| scorer.stop_observing(attempt_id=attempt_id) |
There was a problem hiding this comment.
🔴 Must Fix [P2]: Attempt every observer's cleanup even if one stop fails.
If one stop_observing raises, this loop exits immediately and the remaining observers retain their state. That exception also replaces an execution error or cancellation already in flight. This is a public, pluggable protocol, so cleanup is not limited to the dictionary deletion used by FileWriteScorer.
I reproduced this with a file objective scorer and a file auxiliary scorer. Raising OSError from the objective scorer's stop leaves the auxiliary scorer's baseline in _baselines after the attack exits. A successful execution also loses its saved AttackResult because cleanup raises before result persistence.
Please attempt all registered stops and preserve the original error or cancellation while reporting cleanup failures. The existing objective-target conversation lifecycle above already follows the all-resets cleanup pattern. Add tests for a failing first stop with another observer still pending.
There was a problem hiding this comment.
Fixed. _stop_observers tries every stop once, logs a failure and doesn't re-raise, so the original error or the saved result stands. Tests cover a failing first stop with an auxiliary observer still pending, on both a successful and a failed run.
| return [ | ||
| (scorer, expectation) | ||
| for scorer, expectation in routed | ||
| if isinstance(scorer, AttemptObserver) and expectation is not None |
There was a problem hiding this comment.
🔴 Must Fix [P1]: Start observation for wrapped scorers too.
This only checks the objective and auxiliary root scorers. TrueFalseCompositeScorer, TrueFalseInverterScorer, and fallback wrappers do not implement AttemptObserver, so their FileWriteScorer children never record a baseline and silently use snapshot scoring.
I reproduced this with an unchanged file containing marker and a target that writes nothing:
leaf = FileWriteScorer(source=LocalFileSurfaceSource(root=root))
wrapped = TrueFalseCompositeScorer(
aggregator=TrueFalseScoreAggregator.AND,
scorers=[leaf],
)Using leaf as the objective scorer gives FAILURE with a baseline. Using wrapped gives SUCCESS without a baseline. Wrapping the same scorer should not change whether an earlier run's content counts as this attempt's write.
Please propagate the lifecycle through scorer trees with each child's routed expectation, and add an attack-level regression for a wrapped file scorer.
There was a problem hiding this comment.
Fixed. _attempt_observers now walks the scorer tree through each wrapper's _get_child_expectations, so the leaf gets the expectation its wrapper routes to it. Your unchanged-marker case is parametrised over composite, inverter and fallback, and each comes back FAILURE with a baseline on the stored observation.
| after = _full_text(entry) | ||
| if before is None or after is None: | ||
| return None |
There was a problem hiding this comment.
🔴 Must Fix [P2]: Do not require decoded text when there is no text matcher.
With ContentWritten(matcher=None), a nonempty file with a changed full digest proves a content change under the same exclusive-workspace assumption used elsewhere. This branch still requires complete UTF-8 text before reaching the no-matcher case below. Existing binary files, and existing files beyond max_content_bytes, therefore become UNDETERMINED even when both full hashes show a change.
I reproduced this in a real attack by changing the file from b"\xff" to b"\xfe" and using ContentWritten(uri="/data/out.txt"). The source reads both files completely and their digests differ, but the attack returns UNDETERMINED because before and after are None.
Please handle the no-matcher case after establishing changed is True and a nonempty final file, before requiring decoded text. Keep the text comparison for matchers that need it, and add binary and content-retention-limit regressions.
There was a problem hiding this comment.
Fixed. With no matcher, a changed digest on a nonempty file counts as written before any text is needed. Added your b"\xff" to b"\xfe" case and one past max_content_bytes, both at attack level and in the matcher unit table.
- Observation starts after setup, inside the strategy's execution, so a failure to start records the attempt's ERROR result under its allocated ID. - Every observer is stopped even when one stop fails. A failed stop is logged and no longer replaces the attempt's error, cancellation or saved result. - The attack walks wrapped scorers (composite, inverter, fallback and other wrappers), so a FileWriteScorer inside one records its baseline with the expectation its wrapper routes to it. - A baseline cannot tell overlapping attempts apart. The docs keep the one-at-a-time requirement for a shared root, and FileWriteScorer leaves a change undetermined when another attempt observed the same location at the same time. SurfaceBaseline.concurrent carries this, so replay agrees. - Without a text matcher, a changed nonempty location is credited from its digest, so binary files and files beyond max_content_bytes are no longer undetermined. Each has a regression test.
This branch has not been deployed
3 participants
Phase 11 of the scorer roadmap, built on the merged phase 10 (#3042).
A snapshot shows that content is present, not that this attempt wrote it. Today that is handled by giving each attempt its own workspace. This adds an optional lifecycle so attempts can also share one: the source records a baseline when the attempt starts, and scoring finalizes against it.
SurfaceBaseline, carried on a finalizedSurfaceObservationPayload, so replay gives the same verdict.SurfaceLifecycleSourceandAttemptObserverruntime protocols.LocalFileSurfaceSourceimplementsstart_observing_asyncandfinalize_asyncon the same bounded, confined read.FileWriteScorercredits a location only when the attempt changed it and added the content. Unchanged is a proven negative, identical bytes rewritten stay undetermined, and an append counts only for a match reaching into the new text. Sources without lifecycle support keep the snapshot behaviour.Tests: 103 in test_file_write_scorer.py, including attacks over a shared workspace, release on success and on failure, replay, and Contains, Equals and Regex appends. The doc page gains a shared-workspace example.