Skip to content

fix: emit a note from _write_memory_on_failure - #115

Closed
andrewwhitecdw wants to merge 1 commit into
Agent-Field:mainfrom
andrewwhitecdw:andrewwhitecdw/fix/failure-memory-note-fn
Closed

fix: emit a note from _write_memory_on_failure#115
andrewwhitecdw wants to merge 1 commit into
Agent-Field:mainfrom
andrewwhitecdw:andrewwhitecdw/fix/failure-memory-note-fn

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Closing this stale PR. Happy to reopen if the maintainers would like to revisit it.

_write_memory_on_failure accepted a note_fn argument that every caller
passed, but it never invoked it. As a result, failure-memory updates
were silent. Emit a note so observers can see when failure patterns are
recorded.

Adds a regression test that asserts note_fn is called when failure
memory is written.

Signed-off-by: Andrew White <andrewh@cdw.com>
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@AbirAbbas AbirAbbas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code verified: note_fn was already threaded through all four call sites as an unused parameter, so wiring it up changes no signatures, and the new note only fires after the memory_fn is None guard — consistent with _write_memory_on_approve. The new test fails correctly on unpatched main. Approving the code.

Merge is held on the CLA — please sign via the CLA-assistant link above and this can land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants