Repository navigation
Conversation
sl-7qx
reviewed
Sep 22, 2026
sl-7qx
left a comment
sl-7qx
left a comment
Collaborator
There was a problem hiding this comment.
I found three issues that should be fixed before merging: normal edits are missing from the corrective trace, and two Experience paths bypass existing privacy protections. I reproduced each issue with focused local tests. This was a targeted review, not a full project verification run.
Collaborator
|
weiddeng
requested changes
Sep 27, 2026
weiddeng
left a comment
weiddeng
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the substantial work here and for addressing the initial review. I did another end-to-end pass across corrective capture, distillation, promotion, and retrieval. I found five remaining correctness gaps that affect the feature's core guarantees and should be resolved before merge. Four are noted inline; I added the remaining protection-race detail to the existing privacy thread so that discussion stays together.
al2wang
force-pushed
the
alex/verified-self-evolving-experience
branch
from
October 1, 2026 01:51
06c543a to
655a9a6
Compare
An agent that corrects itself inside a turn currently leaves nothing behind: the recovery is in the trace, and the next turn starts from the same footing. This adds the durable half of verified experience memory. An experience record holds the lesson, the bounded evidence it was derived from, the actor and project it belongs to, its lifecycle status and the decision that last moved it; an evaluation record holds the immutable outcome of assessing one. Records are scoped like every other row here, so one actor's memory is never visible to another, and a record starts as a quarantined candidate that nothing retrieves. Two migrations follow main's current high-water mark, 0052. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Verified experience memory is off unless a deployment asks for it, and promotion is manual unless it asks for that too. Both settings are validated at load time with the rest of the daemon configuration, so an unknown value fails the start instead of silently disabling a control. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Experience must be derived from what the agent actually did, not from the prose it produced. The execution loop now keeps a bounded corrective trace: the verifier that failed, the edits that followed and the same verifier passing afterwards, with every captured string length-bounded and the raw arguments kept as a digest. The context engine gains the item kind those experiences enter a prompt under, so a retrieved lesson is always distinguishable from source evidence and from instructions. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
This is the working half of verified experience memory, behind the configuration added earlier and off by default. After a turn completes, the daemon extracts the bounded corrective evidence the execution loop recorded and stores a quarantined candidate. A separate, tool-free model call distils that evidence into one short reusable lesson and the condition it applies under; the distiller sees only the bounded evidence object, never the workspace, the trace or the model history, and its output is length-bounded, screened for secrets, repository-specific references and unsafe advice, and falls back to a deterministic evidence-derived lesson when it fails. Promotion is explicit. A candidate becomes approved only through a scope-checked decision by the owning actor, and under evaluated promotion only when an eligible evaluation is on record: evaluation evidence is immutable, strictly validated, pre-registered at five repeats per arm, and its gates are recomputed by the daemon from raw counts rather than trusted from the submission. The daemon records and gates; it never runs evaluation jobs. Retrieval is narrow. Only approved, unexpired records owned by the same actor for the same project are read, and only in verified mode. They enter the context as clearly delineated, advisory, derived-untrusted items that never outrank the user, the system rules, policy or tool results, and every retrieval is audited as an event. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Evaluated promotion required a human decision for every candidate, even when the evidence already satisfied every pre-registered gate. A deployment that trusts its evaluation protocol had no way to say so. Automatic promotion is a third, opt-in setting. Under it the daemon promotes a candidate only when an evaluation it already accepted is eligible on its own recomputed gates, and it does so transactionally: the decision, the audit event and the evaluation binding are written together or not at all, so a crash cannot leave an experience approved without the evidence that approved it. Manual and evaluated promotion are unchanged, and nothing promotes without evidence under any setting. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Distillation runs after a turn completes, as best-effort background work. A graceful shutdown cancelled that work at an arbitrary point, so a candidate whose evidence had already been extracted could vanish without ever being written, and nothing in the logs said it had. Post-turn experience work is now started through a registry the server drains on a graceful shutdown, bounded by the distillation deadline plus the same few seconds the rest of the drain allows. Once shutdown has begun the registry refuses new work instead of leaking it. A forced termination still loses best-effort work, as it always did; a graceful one no longer does. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Sessions now carry a mode, and Chat refuses a workspace. The experience lifecycle tests all work against a workspace, so they say Work explicitly rather than leaning on a default. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
The corrective trace read a top-level `path` from an editing call, which is the legacy single-file form. The editing tools now send `files[]` for Lines and Text edits and `patch` with `revisions` for Patch edits, so a perfectly normal repair recorded no edit at all, and verifier-fail -> edit -> verifier-pass never became a recovery segment. The observation boundary now reads the runtime's own report rather than the request: a successful call records the paths its result names, and a batch that stops part-way records exactly the files the error says were applied. Paths that were asked for but not written are recorded as unsuccessful, never as writes; a failure that names no applied file records no write at all. Paths are deduplicated in a deterministic order and stay under the same bounds as the rest of the captured evidence. Tests cover the current `files[]` form, the Patch form, the legacy single path, a partial batch, an opaque failure, and the end-to-end case: a verifier failure, a current-format edit and the same verifier passing now yields corrective evidence naming the edited file. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
A file-protection change is a context reset: `collect_context_items` refuses cached instructions, memory and editor diagnostics across it, because content acquired before the change has no trustworthy provenance afterwards. Verified experience memory is content of exactly that kind, and retrieval did not share the boundary: an approved row kept re-entering the prompt after the reset, carrying whatever it had learned from the files that are now protected. Retrieval now applies the same boundary the rest of the history does. A record acquired at or before the reset is not offered; one acquired after it is, so ordinary use after a protection change keeps working. No new privacy model is introduced, and nothing about file protection is weakened: the existing cutoff is simply respected in one more place. The reviewer's reproduction is a test: an approved Experience carrying a marker reaches the provider before the reset, does not reach it afterwards, no retrieval event claims otherwise for that turn, and an Experience acquired after the reset is retrieved normally. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Distillation received the raw provider, so the one model call the mechanism makes on its own behalf skipped the checks every other call goes through: the current file protections were not applied and the dispatch left no trace in the privacy record. It now goes through the same observed provider as the agent's own calls, under its own purpose, `experience_distillation`. Because the provider checks at stream time, the boundary that applies is the one in force when the background task dispatches - not the one that held when the turn ended. Refusal is fail-closed. If protection moved after the turn, the evidence is not distilled, not screened and not stored at all: the deterministic fallback lesson would otherwise carry the same material past the reset that refused the dispatch. The user's turn is untouched either way, since this work only ever ran after the turn completed. Tests: a permitted distillation dispatches and stores its distilled lesson under the right purpose; a protection change before dispatch leaves the raw provider with no distillation request, persists no lesson, and leaves the completed turn exactly as it was. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
A recovery can straddle the tool-call-limit pause: the verifier fails, the edit lands, the budget runs out, the user resumes, and the same verifier passes. The resume path started the new runner with an empty corrective trace, so the turn ended holding only the final success. Extraction needs the failure and the edit that preceded it, so the episode produced no Experience at all. The checkpoint already carries the trace, exactly as it does for an approval or a question resume; the tool-call-limit resume now hands that trace to the resumed runner instead of a fresh one. Nothing about the pause, the question, the budget or the resume semantics changes. The regression drives the whole path through the API: one verifier run fails, an edit creates the file it checks, a batch trips the limit, the user resumes, the same verifier passes, and the completed turn must still yield evidence naming the pre-pause failure and the edited path. It fails on the previous behaviour with only the post-pause success on record. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
al2wang
force-pushed
the
alex/verified-self-evolving-experience
branch
from
October 1, 2026 19:13
0a2844a to
b2c964c
Compare
Main threaded `transient_tool_media` through the resume and turn-execution signatures while this branch threaded `corrective_trace` through the same ones, so every conflict is the same shape: keep both fields, in both the constructions and the destructuring, and keep main's post-resume media discard. The distillation request carries no tool media — it sends the sealed evidence object and nothing else. No behaviour of either side is changed: a resumed turn still continues its corrective observations, and a resumed approval still discards the cached transient media once the resume succeeds. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
The same integration point as the request itself: this test asserts that the observed provider refuses after a protection reset, and its probe request now carries the empty tool-media map main's ModelRequest requires. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
An approved tool call executes out of band, after the agent loop has already paused and returned its checkpoint, so the loop never sees the result and never observes it. A recovery whose verifier and edit both went through approval therefore left an empty corrective trace and no experience evidence at all, which is exactly the shape a human-supervised session has. The resume now observes that one call itself, from the arguments the checkpoint recorded when the approval was requested and from the outcome the executor actually produced: * The checkpoint carries the approved arguments, so the observation is the paused turn's own call rather than whatever a later record holds. * The checkpoint also names its tool call, and a resolution for any other call of the same turn no longer resumes it. A turn can leave more than one call awaiting approval, and delivering the wrong result under the checkpointed model call id would both mislead the model and observe the wrong command. * The observation follows the executor's report, not the message the model receives: expired transient media is reported to the model as a retryable error while the call itself succeeded. * A denial executes nothing and is not observed. A rejected verifier is not a verifier failure and a rejected edit is not an edit, so neither can become the failure half or the recovery half of evidence. * The trace mutated is the decoded checkpoint's own copy, so a resume that somehow ran twice would still append exactly once. Three regressions fail before this change: an approved failure-edit-pass recovery now yields evidence with exactly one observation per approved call; a denied verifier followed by a real edit and a real pass stays unevidenced; and an approved edit that the executor rejected is observed as not written and never joins the edited paths. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
A result too large to inline is replaced by a handle to a stored artifact, and a handle carries no exit code. The corrective trace classified the handle instead of the result it stands for, and an absent exit code reads as a failing one, so every externalised success was observed as a verifier failure. A large success, an edit, and a small success of the same command therefore looked exactly like a repaired failure, and the extractor had no way to tell that nothing had ever failed. The raw result is now classified before it is externalised: `trace_classification_fields` names the parts of a result that `CorrectiveTrace::observe` reads -- an exit code, an error string, and the paths an edit reports writing -- and the handle carries those forward. Captured output and file contents stay in the artifact; the handle gains only bounded, non-content fields, and the model can now also tell whether a large command succeeded. The agent-core unit test shows the classification of a handle built this way is identical to the classification of the result it replaced, and that no edited content travels with it. The daemon regression runs a verifier whose output is too large to inline, shrinks the file it reads, and runs the same verifier again: before this change the trace held a failure, an edit and a pass, which is admissible evidence for a recovery that never happened; now both runs are successes and nothing is extracted. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
… under The file-protection check that admitted a trajectory ran once, at the top of the recording path, and nothing checked again before the candidate was written. Two windows followed from that. A reset that landed while the distiller's request was in flight left the dispatch admitted and the lesson distilled from material the reset had since withdrawn, and that lesson was persisted. A reset that landed between the admission check and the dispatch made the observed provider refuse the request -- and a refused distillation fell back to the deterministic lesson, which is built from the same evidence, so the material the reset refused to send was stored anyway. Evidence is now bound to the boundary it was admitted under. The recording path stamps the boundary -- the protection revision and its cutoff -- onto the evidence before anything is measured or sent, and `record_experience_evidence` takes it as an argument, so a caller cannot omit it. Immediately before the candidate is written, the boundary in force is read again and must still be that one. Revisions only increase, so an inequality is a reset and not a transient read. An invalidated trajectory is discarded, never downgraded: the fallback lesson is not stored either, because it carries the same evidence. The revalidation is a point-in-time read, so no lock is held across the remote request and a reset still never waits on a model dispatch. The budget measures the widest boundary a record can carry rather than the one in force, so how many resets an actor has made cannot change what fits. Three regressions fail before this change: a reset mid-request discards the distilled lesson it produced; the same reset followed by a failed request stores no fallback lesson either; and a reset between admission and dispatch leaves nothing persisted and nothing dispatched. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
An evaluated-mode approval selected the newest evaluation, validated it, and then transitioned the candidate's status in a separate statement. An evaluation submitted between those two steps superseded the evidence the approval was granted on, so an older pass could mask a newer failure -- which is the one thing selecting the newest evaluation exists to prevent. The decision is now one `BEGIN IMMEDIATE` transaction in storage: the candidate check, the newest-evaluation selection, the named-id and eligibility validation, the durable binding and the status transition. The handler still previews the decision without a write transaction, so a request that is already refusable is refused without one, but the preview is advisory and the transaction decides. Both render their refusals through the same function, so they cannot explain themselves differently. The approval event named the evaluation, but it is published after the decision commits: when that publication failed, an approved -- and so retrievable -- lesson was left with nothing on record saying what approved it. `experiences.approval_evaluation_id` now carries that binding, written in the same transaction as the transition by both the explicit approval and automatic promotion, so no evidence-gated approval can be read without the evidence that granted it. Two regressions fail before this change. A barrier holds an approval between its preview and its decision while a regressed re-run lands in exactly that window: the approval is refused as superseded, the record stays a candidate, and nothing is announced -- before, it was approved with a 200. With the binding dropped from the transition, an approval whose audit event fails leaves an approved record with no binding and no retrievable lesson that explains itself. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
"Only a graceful shutdown is drained" reads as a promise that a graceful shutdown loses nothing. It does not: a task still pending when the twenty-second bound expires is abandoned, along with the candidate it would have recorded, and the only record is the log line the shutdown writes. What the bound guarantees is that the loss is bounded and visible. Whether to replace the bound with durable, resumable work is a separate maintainer decision; this change only stops the documentation from claiming more than the code does. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
`clippy -D warnings` rejects the decision enum: the approved variant carries both records and the refusals carry almost nothing, so the records set the size of every variant. The two records are boxed. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
The daemon stores a turn's completed status before it publishes the turn's usage and terminal events. The helper returned as soon as the stored status read completed, so every test that then reads the event trail could race the publication: under six concurrent runs of the suite this failed oversized_evidence_never_reaches_the_distiller, distilled_experience_is_quarantined_until_approved_then_retrieved_with_audit and distillation_provider_error_and_timeout_fall_back_without_failing_turns, each with a usage or event assertion that read a trail the daemon had not finished writing. The helper now waits for the stored status and for the turn's own terminal event, and its bound is raised to thirty seconds so a loaded runner does not turn a slow turn into a failure. This is the same repair already made on alex/experience-distillation as 704f8c3, which this branch predates. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
An approval, a rejection and an evaluation each leave an audit record, and each of those records was published after the decision's transaction had committed. When that publication failed, an approved -- and therefore retrievable -- lesson was left with nothing on record saying what approved it. The binding added earlier fixed what the record would have said; it did not make the record exist. The record now commits with the decision. The daemon sequences and chains the event before the transaction and hands it in; storage writes it with `insert_audit_event` inside the same `BEGIN IMMEDIATE` transaction as the candidate checks, the evidence binding and the status transition. This covers the evaluated approval, automatic promotion -- which owes two records, at consecutive sequence numbers -- and an ordinary manual decision. Three properties follow, and each is tested: * The record cannot describe a decision that was not made. It names the evidence, and the transaction refuses unless the evidence it selects is that one. One timestamp serves the row and the record. * A rollback leaves neither a gap nor an orphan in the hash chain, because the event ordering is held across the transaction and released without advancing when the transaction fails. * Subscribers are notified after the commit, and that notification is a broadcast whose failure is ignored by construction, so a decision that committed correctly is never undone because nobody was listening. A restart reads the sequence and the chain head back from the database, so a decision that committed before the process ended is recovered with its own record and is refused rather than decided again. No pending state and no queue: with one transaction there is nothing to recover or finalise later, so there is no window in which an approved row is unusable, nothing to resurrect, and no second decision to make idempotent. The approval-binding migration moves to 0057. The shared-skill layer already holds 0055 and 0056 in development databases; two migrations at one version is not a build error but makes every `Store::open` fail with SQLite 1555, and renumbering that layer instead gives an applied database `VersionMismatch(55)`. A database that applied 0057 first accepts 0055 and 0056 afterwards, which is measured. Four regressions fail before this change: a forced failure of the audit insert inside the transaction rolls the approval back with nothing retrievable; a committed approval survives a restart with its original record and refuses the retry; automatic promotion commits both records or neither; and a manual approval without its record does not take effect. The documentation and its mirror now also say what the bounded post-turn drain guarantees: the wait is bounded, not the loss. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Collaborator
Author
Thanks — I agree that this is bounded best-effort work rather than a durability guarantee. The |
…ible The final file-protection check before an Experience candidate is written was a point-in-time read, and the insert was a separate operation. A protection reset could commit between the two; the candidate then received a post-reset created_at, so evidence from the old boundary passed the timestamp-based retrieval filter as if it were fresh. Every protection replacement commits under the per-owner protection gate's exclusive guard (Store::replace_file_protection is the only writer). The recording path now holds that gate's shared guard from the final boundary check until the insert has committed, and releases it before the experience.created fan-out. A reset therefore either commits first, and the check discards the evidence, or waits until the row is durable. The guard never spans the distiller's request or stream. Both the distilled lesson and the deterministic fallback go through this one critical section. A reset's cutoff now also covers this actor's committed experiences, as it already covers turns, inputs, tasks and goals, so a row that won the race stays behind the new boundary even if the wall clock stepped backwards. Regressions use test-only pause points rather than sleeps: a reset landing before the final check discards both distilled and fallback lessons, and a reset requested while the insert is paused after the check is shown queued on the gate, commits only after the insert, and leaves the approved candidate out of a later scripted provider request, while a post-reset experience is still retrieved. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: al2wang <lightever131@gmail.com>
A completed call was turned into AgentToolResult::Failed when publishing its event or externalising its large result failed afterwards, and the agent loop then recorded that delivery error as a failed verifier. A pass, a delivery failure, an edit and a pass of the same command looked like a repaired failure, and could become an experience candidate. The corrective trace now reads only a separate, bounded execution outcome (agent-core ToolExecution: NotExecuted, Unknown, Completed with the trace classification fields and the bounded failure excerpt, or Failed with the executor's own error). AgentToolExecutor gains execute_prepared_observed, which returns it next to the delivered result; the default keeps the old reading for executors whose delivery cannot fail after a call ran. The daemon fixes the outcome from the execution service's report before it publishes or externalises anything, keeps it through every later delivery error, records nothing for vetoed, refused or approval-pending calls, and records nothing when submission failed without a report. The model still sees "tool event publication failed" or the artifact error. Approval resume observes the same outcome. When publishing an approval's outcome fails, the decision and the call have already happened, so the paused turn now resumes with the delivery error as the model-facing result and the real outcome in the trace, instead of being stranded; the HTTP caller still gets the error. Regressions inject one-shot publication, externalisation and approval publication faults through the real runner and executor: a passing verifier stays a pass and yields no candidate; a real failure keeps its own excerpt and its repair is still evidence; an unexecuted verifier and a denial leave nothing; written and partially written edits keep exactly the paths they wrote. Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed run_command carries no exit status; its status reaches the daemon only in a completed result. Almost every failed command never started: argument parsing, file protection, program resolution, executable and shebang trust, staging, sandbox setup and the spawn itself all return ToolCallOutcome::Failed before a process exists, and the remaining wait, capture and drain errors leave the status unknown. Recording such an error as a verifier failure turned a spawn failure, an edit and the same verifier passing into a recovery that never happened. A failed run_command is now an unknown execution and leaves no observation, with one exception: the exact "process timed out" error, which the runtime raises only after killing a process that was running, stays a verifier failure. Failed edits keep their error and so the files a stopped batch wrote. The execution crate is unchanged. The regression fails the first spawn through a one-shot platform runtime, exactly as the native runtime reports a spawn error, then edits and runs the identical verifier for real: the turn completes, the model still sees the spawn error, and no failure or recovery is recorded. A unit test pins the timeout message to the error chain that produces it. Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
al2wang
added a commit
that referenced
this pull request
Oct 11, 2026
Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
al2wang
added a commit
that referenced
this pull request
Oct 11, 2026
…n branch Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
al2wang
added a commit
that referenced
this pull request
Oct 11, 2026
Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
@weiddeng Thanks, both remaining gaps were reproducible. Replying to your 2026-10-11 review of 1. Privacy persistence: fixed in
|
A failed run_command carries no exit status, and the execution path gives no typed signal that its process ever started: a spawn error, a sandbox setup error and a timeout kill all arrive as the same untyped failure. The previous change still recorded a timeout as a verifier failure by matching its exact error message. Error text is not evidence of execution, so a failed command is now always unknown and leaves no observation; a timeout, an edit and the same verifier passing are no recovery. A command that ran and exited nonzero is a completed result with an exit status and stays a verifier failure, whatever its output says. Adding a typed timeout signal would need a new runtime error variant carried through the tool runtime and stored on the tool call, so it is left out. Regressions cover the approval contract through the real HTTP path: - A tool.completed, approval.resolved or tool.denied publication failure happens after the decision and the outcome are committed. It resumes the turn once with the delivery error and observes the outcome once. - Repeating the resolution, or repeating it after a restart that reloads the database, neither executes, observes nor resumes again. - A durable write failure, forced with a trigger on the approval decision or on the executed call's outcome, never resumes the turn and observes nothing. sqlx becomes a daemon dev-dependency for that trigger. Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
weiddeng
previously approved these changes
Oct 12, 2026
weiddeng
left a comment
weiddeng
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed current head 04a7094 against the two requested invariants. The privacy-boundary check and candidate persistence are now serialized against protection replacement with deterministic race coverage, and corrective tracing now follows the actual bounded execution outcome rather than later event or artifact delivery failures. No remaining blockers within this review scope; required CI should continue to gate merge.
al2wang
added a commit
that referenced
this pull request
Oct 12, 2026
…esume contract tests) into the Skill Shop branch Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
al2wang
added a commit
that referenced
this pull request
Oct 12, 2026
…anch Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
@weiddeng One narrow follow-up on item 2, pushed as
Local validation on GitHub CI: run 38189221818 passed on Your re-review request from my previous reply still stands; I'm not re-requesting. |
al2wang
added a commit
that referenced
this pull request
Oct 12, 2026
Signed-off-by: al2wang <lightever131@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
al2wang
added a commit
that referenced
this pull request
Oct 12, 2026
The Skill Shop branch was based on #100 before its review. Twenty-two review commits moved underneath it, and this merge brings them in. Conflicts, resolved: * `AppState`'s builder: the shop's `with_skill_shop` and `with_skill_shop_registry` and the review's test-only `experience_decision_barrier` are independent additions; both kept. * `maybe_resume_turn`: the shop carried an older variant of the approval-resume observation. The reviewed one supersedes it -- it classifies the approved call's execution before anything is externalised, guards the checkpoint's tool call id, takes the approved arguments from the checkpoint rather than from the message history, and carries transient tool media -- so the function is now byte-identical to #100's, and the shop's orphaned `checkpointed_tool_call_arguments` helper is gone. `skills.rs`, ported to the reviewed APIs: * publishing cites the evaluation the approval was bound to (`approval_evaluation_id`), falling back to the newest eligible one for a record approved before bindings existed, and refuses when neither is eligible; * `ExperienceEvidence` gained a privacy boundary; * the test helpers write evaluations and decisions through the audit path, as production does, and share the test's own `AppState` so one sequencer owns the chain -- two states seeded at zero on one store collided on `audit_events.sequence`. `Cargo.lock` is refreshed minimally: the workspace now carries two `sha2` versions and two entries needed the version to disambiguate, which is what `cargo ... --locked` had been failing on in CI. Gate on the merged tree: `cargo fmt --all -- --check`, `cargo clippy --locked --workspace --all-targets -- -D warnings`, and the daemon's experience (15) and skill (19) suites pass. Three approval-resume tests fail here exactly as they do on pristine #100 on this host -- they need a sandbox-capable runner, and #100's CI passes them. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
al2wang
added a commit
that referenced
this pull request
Oct 12, 2026
Brings #100's review, already merged into the Skill Shop branch, down one level. Two conflicts, both additive: `AppState`'s builder keeps this branch's `with_skill_shop_lineage` beside the review's test-only decision barrier, and the empty-evidence literal the review extended had already been removed here, so this branch's side stands. Two test helpers that still took a bare store now take the test's `AppState`, for the reason recorded upstream: one audit sequencer per store. Gate on the merged tree: `cargo fmt --all -- --check`, `cargo clippy --locked --workspace --all-targets -- -D warnings`, daemon experience (15) and skill (22) suites pass. The same three approval-resume tests fail on this host as on pristine #100, and pass in CI. AI-assisted drafting was used; the resulting changes were reviewed and verified by the author. Signed-off-by: al2wang <lightever131@gmail.com>
Collaborator
|
Could we keep PR #100 as the standalone Experience change that was already reviewed and restore its head to I verified that this commit still merges cleanly into current Please also leave out |
Collaborator
Author
|
Thanks, that makes sense. The child PRs got propagated into #100 unintentionally while I was updating the stacked branches. I’ll restore #100 exactly to |
al2wang
force-pushed
the
alex/verified-self-evolving-experience
branch
from
October 12, 2026 22:04
5bca789 to
04a7094
Compare
Collaborator
Author
|
@weiddeng Thanks for catching this. #100 is restored exactly to Required CI on the restored head (run 38314503799): every job passed except
One disclosure for this scope: two approval tests added in Edited: corrected the CI statement above. Linux tests did not run in this run. |
Collaborator
Author
|
Closing and immediately reopening only to trigger a fresh CI run against current |
Collaborator
Author
|
Thanks, #126 has landed, and #100 remains exactly at Fresh required CI against current #101, #109 and #111 remain outside this PR. Re-requesting review of exactly |
weiddeng
approved these changes
Oct 14, 2026
weiddeng
left a comment
weiddeng
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed the restored head 04a70949 against the previously agreed merge scope. The standalone Experience change excludes #101, #109, #111, and 3d4e8b4; the privacy-boundary serialization and execution-result tracing fixes remain intact. Fresh required CI against current main is fully green. No remaining blockers within this review scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds the first landing slice of S-Code's self-evolving agent stack:
corrective execution → distilled Experience → evaluation → approval → verified reuse
The feature is local-first, scoped to the actor and project that produced the Experience, and
off by default. Shared Skills, the Skill Shop, the Forum and all research results are
deliberately out of scope and live in follow-up PRs (#101 → #109 → #111); this PR's review
surface is the verified local Experience memory only.
When an agent runs a verifier that fails, edits the workspace, and later passes the identical
verifier, the daemon preserves that recovery as bounded corrective evidence. After the turn
completes it may distil that evidence into a short reusable Experience candidate. Candidates stay
quarantined until an explicit decision admits them — manually, or through an immutable evaluation
on held-out work that the approval is bound to — and only an approved Experience of the same actor
and project is ever retrieved, as advisory derived-untrusted context below the system rules, the
user, policy, permissions, the sandbox and tool results.
What the review changed (all on this head)
files[]and Patchpatch + revisionsformats are observed; a partial batch records only the paths the runtime wroteff9f565edits_are_observed_in_every_current_editing_form,a_partial_batch_records_only_the_paths_the_runtime_wrote693bf84a_privacy_reset_keeps_a_stale_experience_out_of_the_prompta82c1e5distillation_is_observed_with_its_own_purpose,a_protection_change_before_dispatch_refuses_distillation_and_persists_nothing7cec287a_reset_while_the_distiller_streams_discards_the_lesson_it_produced,a_reset_while_the_distiller_streams_is_never_downgraded_to_a_fallback,a_reset_between_admission_and_dispatch_discards_the_evidence421871capproved_recovery_is_observed_exactly_once_and_becomes_evidence,an_approved_edit_is_observed_with_the_outcome_it_actually_had,a_denied_approval_is_never_a_verifier_failure_or_an_edit,a_failed_approval_resume_is_reported_to_clients,corrective_trace_is_seeded_across_a_resumed_turn28b5ba8classification_fields_carry_the_outcome_and_no_content,an_externalised_large_success_is_never_recovery_evidenceBEGIN IMMEDIATEtransactionf21f518,1aba109an_evaluation_landing_during_an_approval_cannot_be_superseded_after_the_fact,newer_ineligible_evidence_supersedes_an_older_eligible_evaluation,an_approval_keeps_its_binding_when_the_audit_event_fails5ec1b02an_approval_rolls_back_when_its_audit_record_cannot_be_written,an_approval_and_its_audit_record_survive_a_restart_before_delivery,automatic_promotion_commits_both_audit_records_or_neitherDurable audit guarantee
An Experience decision is written in one storage transaction with its approval-evaluation binding
and its audit event: the event is sequenced and chained by the daemon first and inserted inside
the decision's own transaction. If the audit row cannot be written the transaction rolls back and no
approved or retrievable Experience remains. Failing to notify an in-memory subscriber after that
transaction is a delivery concern and never undoes the decision.
Post-turn work and shutdown
Candidate extraction and distillation run after
turn.completed, so they never delay or fail aturn. A graceful shutdown gives in-flight Experience work a bounded opportunity to finish — up to
twenty seconds, the fifteen-second distillation deadline plus the server's five-second drain —
and aborts tasks still in flight after that timeout. This reduces but does not eliminate candidate
loss; no persistent recovery queue is introduced in this PR, and forced termination ends the work
without that opportunity.
Storage
crates/storage/migrations: 0053_experiences, 0054_experience_evaluations and0057_experience_approval_binding (the durable approval-evaluation binding). 0055 and 0056
are reserved by #101's shared-skill layer, which stacks on this PR; the skill registry keeps an
independent migration namespace. Validated with the real migrator on disposable databases:
main(eb7bd9f)maindatabase opened by this branchA #101-migrated database opened by this branch's binary is refused with
VersionMissing(55), asexpected for a downgrade. No applied migration file was edited.
Configuration
(
S_CODE_DAEMON_EXPERIENCE_MODE,S_CODE_DAEMON_EXPERIENCE_PROMOTION.)Review fixes (Wei, 2026-10-11 review of
cd44c18)2b37818).record_experience_evidencenow holds the existingper-owner protection gate (
protection_gate(scope), the same in-process lockreplace_file_protectiontakes for writing)from the final check of the turn's original boundary through the candidate INSERT, and releases it before
experience.createdis published. Distillation, stream consumption and event fan-out stay outside it. The distilled anddeterministic-fallback paths share the block.
replace_file_protectionalso places its cutoff after the newest committedexperience for the same owner, so a candidate that wins the race always lands before the cutoff. No migration;
created_atis never adjusted. Tests:
a_reset_before_the_final_check_discards_a_distilled_lesson,a_reset_before_the_final_check_discards_the_fallback_lesson,a_reset_cannot_commit_between_the_final_check_and_a_distilled_insert,a_reset_cannot_commit_between_the_final_check_and_a_fallback_insert,protection::tests::file_protection_activation_covers_committed_experiences.98bae0a,6817ea3,04a7094). The executor records a bounded executionoutcome as soon as the execution service returns, before publication or artifact externalization. Agent core observes that
outcome once per call (
execute_prepared_observed,ToolExecution), and the model still receives the delivery error. Exit 0is a pass; a nonzero exit is a failure (even when the output says "not found"); denied, awaiting-approval, never-run calls and
failed
run_commandcalls without an exit status (spawn errors, timeouts) are not observed. Error text is never read to decidewhether a call ran. Edits keep only actually written
paths. Approval resume observes the real outcome once; if publishing
tool.completed/approval.resolvedfails, the turn nowresumes with that error instead of staying in AwaitingApproval; the approval decision and the execution outcome are durably
committed before any publication, and a failed durable write never resumes. Tests:
a_publication_failure_after_a_passing_verifier_is_never_recovery_evidence,an_externalisation_failure_after_a_passing_verifier_is_never_recovery_evidence,a_real_verifier_failure_survives_a_delivery_failure_and_its_repair_is_evidence,an_unexecuted_verifier_is_never_the_failure_half_of_a_recovery,a_verifier_that_never_spawned_is_never_the_failure_half_of_a_recovery,an_approved_verifier_keeps_its_outcome_once_through_a_publication_failure,a_written_edit_keeps_its_paths_through_a_publication_failure,a_partial_edit_keeps_only_its_written_paths_through_a_publication_failure,a_failed_command_is_a_verifier_failure_only_when_its_process_ran,a_verifier_killed_at_its_timeout_leaves_no_observation,a_shell_that_ran_and_reported_not_found_is_still_a_verifier_failure,an_approval_delivery_failure_resumes_once_and_never_re_executes,a_failed_durable_approval_or_outcome_write_never_resumes_the_turn,a_denied_approval_whose_publication_fails_resumes_unobserved,the_trace_follows_the_reported_execution_not_the_delivered_result,a_completed_execution_keeps_only_bounded_classification_fields.Known limitation (not changed in this PR): two approval tests added in
04a7094(
an_approval_delivery_failure_resumes_once_and_never_re_executes,a_denied_approval_whose_publication_fails_resumes_unobserved)can time out under heavy load because of a pre-existing race, in which a decision that lands before the turn records its pause is
dropped. The fix is a separate, reviewed follow-up and is not part of this PR.
Validation on
04a7094Local, on a Linux compute node with the command sandbox available (bubblewrap), Rust 1.89.0:
cargo fmt --all -- --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo test --locked --workspaceThe new regressions were written first and failed on
cd44c18. The earlier note thatdocs/source-gatewere red isobsolete: CI was green on
cd44c18.04a7094passed GitHub CI against the previous base (run 38189221818). The run against currentmain(which now includes #125) is reported in the review thread.04a7094addssqlxas a daemon dev-dependency only (an existing workspace crate; no new package).Non-goals
No sharing between agents, no registry, no Skills, no challenge/fork/supersession, no demo, no
learned routing or per-attempt allowlists, and no research results. Those are follow-up PRs or
research instrumentation and are not part of this review.
🤖 Generated with Claude Code