feat(memory): add optional Mem0 backend for skill iteration and reflection tracking - #118
feat(memory): add optional Mem0 backend for skill iteration and reflection tracking#118jrauch713-svg wants to merge 4 commits into
Conversation
|
Thanks for exploring persistent memory for SkillOpt. Memory can be valuable if retrieval measurably improves training, but the current design is not safe to merge yet. The primary blocker is consent and data handling: merely having a global Other required changes:
The CLA check is also still incomplete. Please rework this in the original PR if you would like to continue; that preserves your authorship while allowing a safe review. We can re-review once explicit consent, privacy controls, tests, and a real retrieval benefit are present. |
Yif-Yang
left a comment
Yif-Yang
left a comment
There was a problem hiding this comment.
Requesting changes because the current opt-in and data-handling design can upload private skill and patch content without explicit SkillOpt consent, and the integration lacks tests and a retrieval benefit. Please address the detailed maintainer comment above.
|
Hi @jrauch713-svg — following up with a concise reminder of what is still required before this can be considered:
The branch is also conflicting. Do you plan to continue with this direction? If so, please let us know; we are glad to help keep the scope reviewable. |
|
Uh sorry im just seeing these im surmising in doing something wrong and to
stop?
…On Thu, Jul 9, 2026, 11:11 PM microsoft-github-policy-service[bot] < ***@***.***> wrote:
*microsoft-github-policy-service[bot]* left a comment
(microsoft/SkillOpt#118)
<#118 (comment)>
@jrauch713-svg <https://github.com/jrauch713-svg> please read the
following Contributor License Agreement(CLA). If you agree with the CLA,
please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:
- (default - no company specified) I have sole ownership of
intellectual property rights to my Submissions and I am not making
Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
- (when company given) I am making Submissions in the course of work
for my employer (or my employer has intellectual property rights in my
Submissions by contract or applicable law). I have permission from my
employer to make Submissions and enter into this Agreement on behalf of my
employer. By signing below, the defined term “You” includes me and my
employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement Contribution License Agreement
This Contribution License Agreement (*“Agreement”*) is agreed to by the
party signing below (*“You”*),
and conveys certain license rights to Microsoft Corporation and its
affiliates (“Microsoft”) for Your
contributions to Microsoft open source projects. This Agreement is
effective as of the latest signature
date below.
1. *Definitions*.
*“Code”* means the computer software code, whether in human-readable
or machine-executable form,
that is delivered by You to Microsoft under this Agreement.
*“Project”* means any of the projects owned or managed by Microsoft
and offered under a license
approved by the Open Source Initiative (www.opensource.org).
*“Submit”* is the act of uploading, submitting, transmitting, or
distributing code or other content to any
Project, including but not limited to communication on electronic
mailing lists, source code control
systems, and issue tracking systems that are managed by, or on behalf
of, the Project for the purpose of
discussing and improving that Project, but excluding communication
that is conspicuously marked or
otherwise designated in writing by You as “Not a Submission.”
*“Submission”* means the Code and any other copyrightable material
Submitted by You, including any
associated comments and documentation.
2. *Your Submission*. You must agree to the terms of this Agreement
before making a Submission to any
Project. This Agreement covers any and all Submissions that You, now
or in the future (except as
described in Section 4 below), Submit to any Project.
3. *Originality of Work*. You represent that each of Your Submissions
is entirely Your original work.
Should You wish to Submit materials that are not Your original work,
You may Submit them separately
to the Project if You (a) retain all copyright and license information
that was in the materials as You
received them, (b) in the description accompanying Your Submission,
include the phrase “Submission
containing materials of a third party:” followed by the names of the
third party and any licenses or other
restrictions of which You are aware, and (c) follow any other
instructions in the Project’s written
guidelines concerning Submissions.
4. *Your Employer*. References to “employer” in this Agreement include
Your employer or anyone else
for whom You are acting in making Your Submission, e.g. as a
contractor, vendor, or agent. If Your
Submission is made in the course of Your work for an employer or Your
employer has intellectual
property rights in Your Submission by contract or applicable law, You
must secure permission from Your
employer to make the Submission before signing this Agreement. In that
case, the term “You” in this
Agreement will refer to You and the employer collectively. If You
change employers in the future and
desire to Submit additional Submissions for the new employer, then You
agree to sign a new Agreement
and secure permission from the new employer before Submitting those
Submissions.
5. *Licenses*.
- *Copyright License*. You grant Microsoft, and those who receive the
Submission directly or
indirectly from Microsoft, a perpetual, worldwide, non-exclusive,
royalty-free, irrevocable license in the
Submission to reproduce, prepare derivative works of, publicly
display, publicly perform, and distribute
the Submission and such derivative works, and to sublicense any or all
of the foregoing rights to third
parties.
- *Patent License*. You grant Microsoft, and those who receive the
Submission directly or
indirectly from Microsoft, a perpetual, worldwide, non-exclusive,
royalty-free, irrevocable license under
Your patent claims that are necessarily infringed by the Submission or
the combination of the
Submission with the Project to which it was Submitted to make, have
made, use, offer to sell, sell and
import or otherwise dispose of the Submission alone or with the
Project.
- *Other Rights Reserved*. Each party reserves all rights not
expressly granted in this Agreement.
No additional licenses or rights whatsoever (including, without
limitation, any implied licenses) are
granted by implication, exhaustion, estoppel or otherwise.
6. *Representations and Warranties*. You represent that You are
legally entitled to grant the above
licenses. You represent that each of Your Submissions is entirely Your
original work (except as You may
have disclosed under Section 3). You represent that You have secured
permission from Your employer to
make the Submission in cases where Your Submission is made in the
course of Your work for Your
employer or Your employer has intellectual property rights in Your
Submission by contract or applicable
law. If You are signing this Agreement on behalf of Your employer, You
represent and warrant that You
have the necessary authority to bind the listed employer to the
obligations contained in this Agreement.
You are not expected to provide support for Your Submission, unless
You choose to do so. UNLESS
REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING, AND EXCEPT FOR THE
WARRANTIES
EXPRESSLY STATED IN SECTIONS 3, 4, AND 6, THE SUBMISSION PROVIDED
UNDER THIS AGREEMENT IS
PROVIDED WITHOUT WARRANTY OF ANY KIND, INCLUDING, BUT NOT LIMITED TO,
ANY WARRANTY OF
NONINFRINGEMENT, MERCHANTABILITY, OR FITNESS FOR A PARTICULAR PURPOSE.
7. *Notice to Microsoft*. You agree to notify Microsoft in writing of
any facts or circumstances of which
You later become aware that would make Your representations in this
Agreement inaccurate in any
respect.
8. *Information about Submissions*. You agree that contributions to
Projects and information about
contributions may be maintained indefinitely and disclosed publicly,
including Your name and other
information that You submit with Your Submission.
9. *Governing Law/Jurisdiction*. This Agreement is governed by the
laws of the State of Washington, and
the parties consent to exclusive jurisdiction and venue in the federal
courts sitting in King County,
Washington, unless no federal subject matter jurisdiction exists, in
which case the parties consent to
exclusive jurisdiction and venue in the Superior Court of King County,
Washington. The parties waive all
defenses of lack of personal jurisdiction and forum non-conveniens.
10. *Entire Agreement/Assignment*. This Agreement is the entire
agreement between the parties, and
supersedes any and all prior agreements, understandings or
communications, written or oral, between
the parties relating to the subject matter hereof. This Agreement may
be assigned by Microsoft.
—
Reply to this email directly, view it on GitHub
<#118?email_source=notifications&email_token=CFISKZH6W3CFL2PX626I4VD5EBUF3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGE4TANBXGI4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4931904728>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CFISKZB5BQM2Z5ZU7GP7FE35EBUF3AVCNFSNUABGKJSXA33TNF2G64TZHMYTEMZSGY3DSOBZGA5US43TOVSTWNBYGUYTSMRSGQZDPILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CFISKZAPJNWOTXSOKLTCRAD5EBUF3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGE4TANBXGI4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/CFISKZBGST7L6QMVYE5TNUD5EBUF3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGE4TANBXGI4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Not at all — we are not asking you to stop, and you did nothing wrong by exploring this direction. We think a Mem0 integration could be valuable for SkillOpt. Our review only means that the current implementation needs some iteration before it can be merged. The main concerns are that it can upload skill/reflection content without an explicit SkillOpt opt-in, and that it currently writes memories without retrieving and using them to improve training. If you would like to continue, we would be happy to help break the work into smaller, clearer steps:
There is no pressure to do everything at once. Please let us know if you would like to continue, and we can help review each step. |
|
Hi @Yif-Yang — sorry for the slow reply, I missed the earlier
notifications. Yes, I'd like to continue.
I'm a solo developer and this is my first contribution here, so the
detailed review is genuinely helpful. Signing the CLA now.
I agree the write-only design is the core issue — uploading without reading
memory back doesn't justify the privacy and latency cost. I'd rather narrow
the scope than half-fix everything. Would it be reviewable if I focused
first on explicit opt-in + redaction + namespacing, with retrieval
integration as a follow-up? Or would you prefer retrieval in the same PR
since that's what justifies the feature?
Happy to follow whatever sequencing works for you.
…On Thu, Jul 9, 2026, 11:11 PM microsoft-github-policy-service[bot] < ***@***.***> wrote:
*microsoft-github-policy-service[bot]* left a comment
(microsoft/SkillOpt#118)
<#118 (comment)>
@jrauch713-svg <https://github.com/jrauch713-svg> please read the
following Contributor License Agreement(CLA). If you agree with the CLA,
please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:
- (default - no company specified) I have sole ownership of
intellectual property rights to my Submissions and I am not making
Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
- (when company given) I am making Submissions in the course of work
for my employer (or my employer has intellectual property rights in my
Submissions by contract or applicable law). I have permission from my
employer to make Submissions and enter into this Agreement on behalf of my
employer. By signing below, the defined term “You” includes me and my
employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement Contribution License Agreement
This Contribution License Agreement (*“Agreement”*) is agreed to by the
party signing below (*“You”*),
and conveys certain license rights to Microsoft Corporation and its
affiliates (“Microsoft”) for Your
contributions to Microsoft open source projects. This Agreement is
effective as of the latest signature
date below.
1. *Definitions*.
*“Code”* means the computer software code, whether in human-readable
or machine-executable form,
that is delivered by You to Microsoft under this Agreement.
*“Project”* means any of the projects owned or managed by Microsoft
and offered under a license
approved by the Open Source Initiative (www.opensource.org).
*“Submit”* is the act of uploading, submitting, transmitting, or
distributing code or other content to any
Project, including but not limited to communication on electronic
mailing lists, source code control
systems, and issue tracking systems that are managed by, or on behalf
of, the Project for the purpose of
discussing and improving that Project, but excluding communication
that is conspicuously marked or
otherwise designated in writing by You as “Not a Submission.”
*“Submission”* means the Code and any other copyrightable material
Submitted by You, including any
associated comments and documentation.
2. *Your Submission*. You must agree to the terms of this Agreement
before making a Submission to any
Project. This Agreement covers any and all Submissions that You, now
or in the future (except as
described in Section 4 below), Submit to any Project.
3. *Originality of Work*. You represent that each of Your Submissions
is entirely Your original work.
Should You wish to Submit materials that are not Your original work,
You may Submit them separately
to the Project if You (a) retain all copyright and license information
that was in the materials as You
received them, (b) in the description accompanying Your Submission,
include the phrase “Submission
containing materials of a third party:” followed by the names of the
third party and any licenses or other
restrictions of which You are aware, and (c) follow any other
instructions in the Project’s written
guidelines concerning Submissions.
4. *Your Employer*. References to “employer” in this Agreement include
Your employer or anyone else
for whom You are acting in making Your Submission, e.g. as a
contractor, vendor, or agent. If Your
Submission is made in the course of Your work for an employer or Your
employer has intellectual
property rights in Your Submission by contract or applicable law, You
must secure permission from Your
employer to make the Submission before signing this Agreement. In that
case, the term “You” in this
Agreement will refer to You and the employer collectively. If You
change employers in the future and
desire to Submit additional Submissions for the new employer, then You
agree to sign a new Agreement
and secure permission from the new employer before Submitting those
Submissions.
5. *Licenses*.
- *Copyright License*. You grant Microsoft, and those who receive the
Submission directly or
indirectly from Microsoft, a perpetual, worldwide, non-exclusive,
royalty-free, irrevocable license in the
Submission to reproduce, prepare derivative works of, publicly
display, publicly perform, and distribute
the Submission and such derivative works, and to sublicense any or all
of the foregoing rights to third
parties.
- *Patent License*. You grant Microsoft, and those who receive the
Submission directly or
indirectly from Microsoft, a perpetual, worldwide, non-exclusive,
royalty-free, irrevocable license under
Your patent claims that are necessarily infringed by the Submission or
the combination of the
Submission with the Project to which it was Submitted to make, have
made, use, offer to sell, sell and
import or otherwise dispose of the Submission alone or with the
Project.
- *Other Rights Reserved*. Each party reserves all rights not
expressly granted in this Agreement.
No additional licenses or rights whatsoever (including, without
limitation, any implied licenses) are
granted by implication, exhaustion, estoppel or otherwise.
6. *Representations and Warranties*. You represent that You are
legally entitled to grant the above
licenses. You represent that each of Your Submissions is entirely Your
original work (except as You may
have disclosed under Section 3). You represent that You have secured
permission from Your employer to
make the Submission in cases where Your Submission is made in the
course of Your work for Your
employer or Your employer has intellectual property rights in Your
Submission by contract or applicable
law. If You are signing this Agreement on behalf of Your employer, You
represent and warrant that You
have the necessary authority to bind the listed employer to the
obligations contained in this Agreement.
You are not expected to provide support for Your Submission, unless
You choose to do so. UNLESS
REQUIRED BY APPLICABLE LAW OR AGREED TO IN WRITING, AND EXCEPT FOR THE
WARRANTIES
EXPRESSLY STATED IN SECTIONS 3, 4, AND 6, THE SUBMISSION PROVIDED
UNDER THIS AGREEMENT IS
PROVIDED WITHOUT WARRANTY OF ANY KIND, INCLUDING, BUT NOT LIMITED TO,
ANY WARRANTY OF
NONINFRINGEMENT, MERCHANTABILITY, OR FITNESS FOR A PARTICULAR PURPOSE.
7. *Notice to Microsoft*. You agree to notify Microsoft in writing of
any facts or circumstances of which
You later become aware that would make Your representations in this
Agreement inaccurate in any
respect.
8. *Information about Submissions*. You agree that contributions to
Projects and information about
contributions may be maintained indefinitely and disclosed publicly,
including Your name and other
information that You submit with Your Submission.
9. *Governing Law/Jurisdiction*. This Agreement is governed by the
laws of the State of Washington, and
the parties consent to exclusive jurisdiction and venue in the federal
courts sitting in King County,
Washington, unless no federal subject matter jurisdiction exists, in
which case the parties consent to
exclusive jurisdiction and venue in the Superior Court of King County,
Washington. The parties waive all
defenses of lack of personal jurisdiction and forum non-conveniens.
10. *Entire Agreement/Assignment*. This Agreement is the entire
agreement between the parties, and
supersedes any and all prior agreements, understandings or
communications, written or oral, between
the parties relating to the subject matter hereof. This Agreement may
be assigned by Microsoft.
—
Reply to this email directly, view it on GitHub
<#118?email_source=notifications&email_token=CFISKZH6W3CFL2PX626I4VD5EBUF3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGE4TANBXGI4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4931904728>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CFISKZB5BQM2Z5ZU7GP7FE35EBUF3AVCNFSNUABGKJSXA33TNF2G64TZHMYTEMZSGY3DSOBZGA5US43TOVSTWNBYGUYTSMRSGQZDPILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CFISKZAPJNWOTXSOKLTCRAD5EBUF3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGE4TANBXGI4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/CFISKZBGST7L6QMVYE5TNUD5EBUF3A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJTGE4TANBXGI4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Yes — please continue! We would be very happy to work with you on this, and your instinct to narrow the scope is exactly right. We also appreciate that you identified the write-only design as the central issue. Our preference would be to keep one small retrieval path in this PR, because that is what turns the integration from external logging into a useful memory loop. It does not need to become a large system. A reviewable minimum would be:
You do not need to solve batching, async writes, sophisticated ranking, or run a large benchmark study in this iteration. A deterministic end-to-end test and a small opt-in smoke example are enough for us to review the correctness of the first useful loop; broader performance evaluation can be follow-up work. Please take the time you need, complete the CLA using the option that applies to you, and rebase onto current main when convenient. Feel free to push an incremental version or ask questions before everything is finished — we are glad to help, especially for your first contribution here. Thank you for continuing! |
|
Hi Yifan,
Thank you for the encouragement and for clarifying the next steps for the
PR.
I am currently assisting a resident at the assisted living facility where I
work and preparing lunch, so I am a bit tied up at the moment. I would
appreciate some time to get situated, and I will get back to you as soon as
I can.
Best regards,
Joshua Rauch
…On Sat, Jul 25, 2026, 1:26 PM Yifan Yang ***@***.***> wrote:
*Yif-Yang* left a comment (microsoft/SkillOpt#118)
<#118 (comment)>
Yes — please continue! We would be very happy to work with you on this,
and your instinct to narrow the scope is exactly right. We also appreciate
that you identified the write-only design as the central issue.
Our preference would be to keep one small retrieval path in this PR,
because that is what turns the integration from external logging into a
useful memory loop. It does not need to become a large system. A reviewable
minimum would be:
1. An explicit Mem0 setting that is disabled by default — the presence
of MEM0_API_KEY alone must not enable uploads.
2. Redaction before any outbound request, plus a stable
project-specific namespace.
3. One bounded retrieval call before reflection, with the retrieved
context actually included in the reflection input.
4. Mocked tests showing that disabled mode sends nothing,
redaction/namespacing work, retrieved context reaches reflection, and
service failures degrade gracefully.
You do not need to solve batching, async writes, sophisticated ranking, or
run a large benchmark study in this iteration. A deterministic end-to-end
test and a small opt-in smoke example are enough for us to review the
correctness of the first useful loop; broader performance evaluation can be
follow-up work.
Please take the time you need, complete the CLA using the option that
applies to you, and rebase onto current main when convenient. Feel free to
push an incremental version or ask questions before everything is finished
— we are glad to help, especially for your first contribution here. Thank
you for continuing!
—
Reply to this email directly, view it on GitHub
<#118?email_source=notifications&email_token=CFISKZEBEQX6TK2H54IAT635GT3W5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBXHE4DQNJQHE4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5079885098>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CFISKZCZBBUIXPHG7W2KLM35GT3W5AVCNFSNUABGKJSXA33TNF2G64TZHMYTEMZSGY3DSOBZGA5US43TOVSTWNBYGUYTSMRSGQZDPILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CFISKZFUIAWFRO6N4E5QB3T5GT3W5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBXHE4DQNJQHE4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/CFISKZFJL3TREOQBLSSVBGD5GT3W5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBXHE4DQNJQHE4KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@microsoft-github-policy-service agree |
…ction tracking Wires SkillMemory into ReflACTTrainer via non-breaking hooks: records each step's skill/score after the evaluation gate and each step's reflection patches after the accumulation loop. Degrades to a no-op when MEM0_API_KEY is unset.
…l path Addresses the review on microsoft#118. The integration was previously write-only and enabled by the mere presence of MEM0_API_KEY; both are fixed here. 1. Explicit opt-in. Uploading now requires `mem0_enabled: true`. A key present in the environment for another application no longer enables anything — a disabled run does not even read the key. New `skillopt/memory/settings.py` resolves all `mem0_*` config in one place. 2. Redaction before any outbound request. New `skillopt/memory/redaction.py` is the single choke point: no payload reaches the client without passing through `redact_for_upload`, which strips vendor keys, bearer/basic tokens, JWTs, private keys, `key = value` secret assignments, the project root, and `/home/<user>`-style prefixes. Credentials are scrubbed before paths are collapsed, so a key embedded in a home path cannot survive. Relative paths and filenames are preserved so stored memories remain useful. The patterns deliberately mirror `skillopt_sleep/staging.py` rather than importing it — pyproject keeps that package decoupled with zero research dependency. 3. Stable project-specific namespace. Memories are scoped to `skillopt:<env>:<sha256-prefix-of-project-root>`, which is stable across runs of one project and distinct across projects. The raw path is hashed, never transmitted. Replaces the previous config-name/env user id that could mix unrelated projects. 4. One bounded retrieval call before reflection. `hook_pre_reflect` fetches relevant history and appends it to the reflection input, turning the integration from external logging into a memory loop. It is deliberately assigned to a separate `reflect_context` variable so the autonomous learning-rate decision and the rewrite prompt continue to see the unaugmented context. Also in this commit: - Every call is wall-clock bounded by `mem0_timeout_seconds` (default 5s), so an unreachable service costs one bounded pause instead of blocking a step. - Malformed patches (None, strings, non-lists) are filtered rather than raised inside a swallowing try. - The `mem0` optional extra is declared, deliberately outside `all`. - The `mem0_*` keys are mapped through `_FLATTEN_MAP` under `train.`. Without this they were silently dropped for structured configs — which is every config in the repo, since they all inherit `_base_/default.yaml` — leaving the feature inert with no error. - `configs/features/mem0_memory.yaml` is a documented opt-in example following the `soft_gate.yaml` convention. - The config reference documents every key and exactly what leaves the machine. Tests (`tests/test_mem0_memory.py`, 15 cases, no network) cover: a bare MEM0_API_KEY not enabling uploads, explicit opt-in writing, redaction of secrets and home paths, cap applied after redaction, namespace stability and project separation, structured-config keys surviving flattening, the shipped example config actually enabling the feature, retrieval reaching the actual reflection prompt (verified end-to-end by capturing the optimizer's user message), retrieval being independently disableable, graceful degradation on service failure, timeout bounding, and malformed-patch filtering. Not included, per review guidance: batching, async writes, ranking, and benchmark study are left as follow-up work.
64326a5 to
27d6cd9
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in persistent memory integration for SkillOpt training runs, backing storage/retrieval with Mem0 and wiring it into the ReflACT training loop via hooks that no-op unless explicitly enabled.
Changes:
- Introduces a new
skillopt.memorypackage (settings resolution, redaction, Mem0 backend, and trainer hooks) to store skill iterations and reflection summaries and optionally retrieve relevant context for reflection. - Wires memory hooks into the main trainer loop and extends config flattening plus docs/example config to expose
train.mem0_*parameters. - Adds an end-to-end mocked test suite validating opt-in safety, redaction, namespacing, retrieval plumbing, graceful failure, time bounds, and malformed patch handling.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_mem0_memory.py | Adds comprehensive mocked coverage for opt-in behavior, redaction, retrieval, and failure/timeout handling. |
| skillopt/memory/trainer_hooks.py | Implements maybe_init_mem0 plus pre/post hooks to read/write memory around reflect/evaluate. |
| skillopt/memory/settings.py | Adds strict opt-in settings resolution (never consult env key unless enabled) and namespacing. |
| skillopt/memory/redaction.py | Centralizes outbound redaction for secrets and machine-identifying paths. |
| skillopt/memory/mem0_backend.py | Implements the Mem0 client wrapper with redaction, truncation, bounded calls, and persistence APIs. |
| skillopt/memory/init.py | Exposes the new memory package public API. |
| skillopt/engine/trainer.py | Calls the memory hooks around reflect/evaluate and initializes the backend once per run. |
| skillopt/config.py | Maps structured train.mem0_* config keys into the flattened trainer config. |
| pyproject.toml | Adds a mem0 optional dependency extra (mem0ai). |
| docs/reference/config.md | Documents mem0 memory configuration, safety properties, and what data leaves the machine. |
| configs/features/mem0_memory.yaml | Adds a shipped example config that enables mem0 memory explicitly under train:. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # ── Training loop ──────────────────────────────────────────────── | ||
| t_loop_start = time.time() | ||
|
|
||
| memory = maybe_init_mem0(cfg) | ||
|
|
| except _FuturesTimeout: | ||
| print(f" [mem0] call exceeded {self.settings.timeout_seconds}s — continuing without it") | ||
| return None |
| text = rec.get("memory") or rec.get("text") or "" | ||
| if not isinstance(text, str) or not text.strip(): | ||
| continue | ||
| lines.append(f"- {text.strip()}") |
|
Hi @Yif-Yang — pushed. The branch is rebased onto current 1. Explicit opt-in. Uploading now requires 2. Redaction + namespacing. Namespaces are I mirrored the secret patterns from 3. One bounded retrieval call before reflection. 4. Mocked tests — Also in this push:
Two things I hit while rebasing that are worth flagging:
Full suite: 485 passed, 6 skipped, 0 failed. One honest caveat: all tests are mocked — nothing has been run against the live Mem0 service yet. If you'd like a real smoke run before merging, say so and I'll do it. Left out per your guidance: batching, async writes, ranking, and benchmark evaluation. Thanks again for the detailed review and for breaking it into steps — it made this much easier to approach. |
…ilure, cleanup Three findings from the Copilot review pass on the previous commit. All are real. 1. Redact retrieved text before it enters the reflection prompt. Outbound redaction alone was insufficient: mem0 is an external store that may hold records written by an older version of this code, another tool, or a shared namespace, and anything it returns is forwarded to the optimizer's model provider. Retrieved records are now scrubbed on the way in as well as out, so untrusted store contents cannot leak to a second third party. 2. Bound the cost of a wedged service properly. With a single worker, a call stuck on a socket read left the executor occupied, so every later call queued behind it and still paid the full timeout — the cost recurred on every remaining step and the queue grew without bound. On timeout the future is cancelled and the executor retired, and after three consecutive failures the backend disables itself for the rest of the run. A persistently unreachable service now costs a bounded total rather than a bounded amount per step. A successful call resets the counter. 3. Guarantee the worker thread is released. SkillMemory owned a ThreadPoolExecutor whose non-daemon worker could keep the interpreter alive at exit if wedged. `close()` is now idempotent and cancels pending futures, an atexit hook covers aborted runs, and `train()` closes the backend on the normal path. Five tests added (20 total in tests/test_mem0_memory.py): retrieved secrets and home paths are stripped before reaching the prompt, repeated timeouts disable the backend within a bounded wall-clock budget, the executor is retired after a timeout, close() is idempotent and refuses later calls, and a success resets the failure counter. Full suite: 490 passed, 6 skipped. ruff clean on all new files.
|
Pushed
Five tests added (20 total, still no network): retrieved secrets and home paths are stripped before reaching the prompt, repeated timeouts disable the backend inside a bounded wall-clock budget, the executor is retired after a timeout, Full suite: 490 passed, 6 skipped. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
skillopt/engine/trainer.py:2444
memory.close()is only called on the normal return path. Iftrain()raises mid-run and the caller catches the exception (e.g., in a long-lived process), the mem0 ThreadPoolExecutor thread can remain alive until process exit. Consider wrapping the training body in atry/finallysomemory.close()always runs oncememoryhas been initialised.
# Release the memory worker thread. SkillMemory also registers an
# atexit hook, so an aborted run is covered too; this is the clean
# path so a long-lived process does not hold the thread until exit.
if memory is not None:
memory.close()
skillopt/memory/settings.py:113
resolve_settingsderives the namespace digest fromout_root, butscripts/train.pyauto-generatesout_rootwith a timestamp per run (scripts/train.py:573-580). That makes the namespace (andproject_rootused for redaction) change every run by default, defeating the stated goal of stable, project-specific memory across runs.
project_root = os.path.abspath(str(cfg.get("out_root") or os.getcwd()))
namespace = str(cfg.get("mem0_namespace") or "").strip()
if not namespace:
namespace = project_namespace(project_root, str(cfg.get("env") or ""))
skillopt/memory/mem0_backend.py:168
_payload()treatsmem0_max_chars <= 0as a 0-length limit but still appends the truncation marker, producing a payload that is just a newline + "[...truncated]". If users setmem0_max_chars: 0to avoid uploading text, this still uploads content (and may violate upstream expectations for non-empty text).
limit = max(0, int(self.settings.max_chars))
if len(redacted) > limit:
return redacted[:limit] + "\n[...truncated]"
return redacted
|
Sorry still dont know what im doing lol. |
Yif-Yang
left a comment
Yif-Yang
left a comment
There was a problem hiding this comment.
Thank you for the substantial update. You addressed the original review well: the feature is explicitly opt-in, outbound and inbound content is redacted, retrieval now reaches reflection, the CLA is complete, and the focused suite passes. The Mem0 direction remains valuable.
Our latest review found three remaining correctness/safety issues:
mem0_api_keyis not included inskillopt.engine.trainer._SECRET_KEYS. When supplied in YAML/config rather than only through the environment, the resolved key is currently written intoconfig.jsonin plaintext. Please add it to the central config-artifact redaction path and test bothconfig.jsonand any summary/artifact serialization.- The default namespace is derived from
cfg["out_root"]. In normal usage that identifies an output/run directory, not the project, so changing the run directory produces a different namespace and defeats cross-run retrieval. Please derive it from a stable project/repository identity (or require an explicit namespace) and add a test showing two output directories for the same project resolve to the same namespace. ThreadPoolExecutor.shutdown(wait=False)cannot stop a request already blocked in a worker, and its non-daemon thread can keep Python alive after the apparent timeout. Retiring the executor can leave up to three stuck threads before the circuit breaker opens. The unit test reports quickly because it measures the caller, but a process-level check still waits for the worker to finish. Please enforce the timeout at the Mem0/HTTP client layer, or isolate calls in a terminable process, and add a subprocess test proving a permanently blocked request cannot prevent interpreter exit.
This is no longer a request for a redesign or a benchmark—these are focused fixes to make the implementation safe in real runs. A live service smoke test can wait until after these deterministic issues are resolved. We appreciate the care you have already put into this first contribution and will re-review promptly.
|
Thanks for the feedback! Ill get on that tonight!
…On Wed, Jul 29, 2026 at 4:00 PM Yifan Yang ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thank you for the substantial update. You addressed the original review
well: the feature is explicitly opt-in, outbound and inbound content is
redacted, retrieval now reaches reflection, the CLA is complete, and the
focused suite passes. The Mem0 direction remains valuable.
Our latest review found three remaining correctness/safety issues:
1. mem0_api_key is not included in skillopt.engine.trainer._SECRET_KEYS.
When supplied in YAML/config rather than only through the environment, the
resolved key is currently written into config.json in plaintext.
Please add it to the central config-artifact redaction path and test both
config.json and any summary/artifact serialization.
2. The default namespace is derived from cfg["out_root"]. In normal
usage that identifies an output/run directory, not the project, so changing
the run directory produces a different namespace and defeats cross-run
retrieval. Please derive it from a stable project/repository identity (or
require an explicit namespace) and add a test showing two output
directories for the same project resolve to the same namespace.
3. ThreadPoolExecutor.shutdown(wait=False) cannot stop a request
already blocked in a worker, and its non-daemon thread can keep Python
alive after the apparent timeout. Retiring the executor can leave up to
three stuck threads before the circuit breaker opens. The unit test reports
quickly because it measures the caller, but a process-level check still
waits for the worker to finish. Please enforce the timeout at the Mem0/HTTP
client layer, or isolate calls in a terminable process, and add a
subprocess test proving a permanently blocked request cannot prevent
interpreter exit.
This is no longer a request for a redesign or a benchmark—these are
focused fixes to make the implementation safe in real runs. A live service
smoke test can wait until after these deterministic issues are resolved. We
appreciate the care you have already put into this first contribution and
will re-review promptly.
—
Reply to this email directly, view it on GitHub
<#118?email_source=notifications&email_token=CFISKZBV4YE5464AXT6PM2L5HJQXNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBRGI4DMOBXGM2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4812868734>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/CFISKZBKOW4KGMY5HQB6XUL5HJQXNAVCNFSNUABGKJSXA33TNF2G64TZHMYTEMZSGY3DSOBZGA5US43TOVSTWNBYGUYTSMRSGQZDPILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/CFISKZAPFTVQVTAGA5WBTO35HJQXNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBRGI4DMOBXGM2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/CFISKZALDYOM5XIQQNOLCO35HJQXNA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIOBRGI4DMOBXGM2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…eout
Addresses the three issues from the second maintainer review. All three were
confirmed against the code before fixing.
1. `mem0_api_key` was absent from `trainer._SECRET_KEYS`, so a key supplied in
YAML rather than the environment was written in plaintext to both
`config.json` and `summary["config"]` — both serialise through
`_redact_cfg`. Added to the central redaction set. Doubly worth fixing
because `configs/features/mem0_memory.yaml` offers that field.
2. The default namespace was derived from `cfg["out_root"]`, which
`scripts/train.py` defaults to
`outputs/skillopt_<env>_<model>_<timestamp>`. That minted a fresh namespace
on every run, so cross-run retrieval could never return anything — the
feature was not merely degraded but inert. Identity now comes from the
enclosing git repository root, falling back to the working directory, via
the new `project_identity()`. `out_root` still anchors path redaction, which
is what it is correct for.
3. The timeout was not enforced where the work happens. `ThreadPoolExecutor`
workers are non-daemon and `concurrent.futures` installs an atexit hook that
joins them, so a request blocked in a socket read held the interpreter open
at shutdown regardless of `shutdown(wait=False)`, and up to three threads
could wedge before the circuit breaker opened. Two changes:
- The timeout is pushed into the HTTP layer: `MemoryClient` accepts an
injected `httpx.Client`, so it is constructed with
`timeout=mem0_timeout_seconds` instead of mem0's default of 300s. The
request now aborts rather than being abandoned.
- Calls run on daemon threads, so even a pathologically stuck request cannot
delay interpreter exit. The executor and its atexit hook are gone.
Tests: 22 total (was 20), still no network.
- `test_mem0_api_key_is_redacted_from_config_artifacts` covers `_redact_cfg`
(both artifacts) and asserts the live config is not mutated.
- `test_namespace_is_stable_across_different_out_roots` proves two runs with
different timestamped output directories resolve to one namespace.
- `test_blocked_request_cannot_prevent_interpreter_exit` spawns a real
interpreter, wedges a request, and requires the process to exit on its own.
The previous caller-side assertion passed while the process hung, so the
check had to move to process level.
Full suite: 492 passed, 6 skipped. ruff clean on all new files.
|
Thanks — all three were real, and I confirmed each against the code before changing anything. Pushed 1. 2. Namespace identity. You were right, and it was worse than inert-on-changed-directory: 3. Timeout enforcement. Also correct, and my earlier test was measuring the wrong thing. Two changes:
The test moved to process level as you asked — it spawns a real interpreter, wedges a request permanently, and requires the process to exit on its own. Worth recording that before the fix the caller returned in 0.20s (which my old assertion was happy with) while the process never exited at all; that gap is exactly what you identified. Suite is 22 tests, still no network. Full run: 492 passed, 6 skipped; Happy to do the live service smoke test whenever you'd like it. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
skillopt/memory/mem0_backend.py:103
from_settings()currently returns aSkillMemoryinstance whenclientis provided even if the run did not opt in (settings.usableis false). That contradicts the documented behavior (“None unless opted in”) and makes it easier for callers to accidentally bypass the feature flag.
if not settings.usable and client is None:
return None
if client is None and not _MEM0_AVAILABLE:
return None
return cls(settings, client=client)
docs/reference/config.md:187
- The failure-behavior docs say a timed-out call “retires the worker”, but there is no long-lived worker to retire; the backend abandons a stuck call (daemon thread) and future calls don’t queue behind it. Please adjust this wording to reflect the actual implementation.
A call that exceeds `mem0_timeout_seconds` releases the training step on time and
retires the worker, so the next step does not queue behind a wedged request. After
three consecutive failures the backend stops calling out for the remainder of the run
skillopt/memory/mem0_backend.py:73
SkillMemorycan currently be constructed with a customclienteven whensettings.usableis false, which bypasses the opt-in requirement described in the module safety contract and could allow accidental outbound calls from a disabled config. Enforcesettings.usableregardless of whether a client is injected.
This issue also appears on line 99 of the same file.
def __init__(self, settings: Mem0Settings, client: Any | None = None) -> None:
if client is None:
if not _MEM0_AVAILABLE:
raise ImportError("mem0ai is not installed. Run: pip install 'skillopt[mem0]'")
if not settings.usable:
raise ValueError("SkillMemory requires mem0_enabled=true and an API key")
# Enforce the timeout where the request actually happens. mem0's own
skillopt/memory/mem0_backend.py:172
mem0_max_charsis documented as a cap on the outbound payload, but_payload()returnsredacted[:limit] + "\n[...truncated]", which can exceed the configured limit. If the cap is meant to be strict, the truncation marker should be included withinmem0_max_chars.
limit = max(0, int(self.settings.max_chars))
if len(redacted) > limit:
return redacted[:limit] + "\n[...truncated]"
return redacted
tests/test_mem0_memory.py:190
- This test currently allows the payload to exceed
mem0_max_charsby the length of the truncation marker. Ifmem0_max_charsis intended as a strict cap on the actual transmitted payload, the assertion should enforce that.
sent = client.adds[0][0][0]["content"]
assert len(sent) <= 120 + len("\n[...truncated]")
skillopt/engine/trainer.py:2443
- The comment says
SkillMemory“registers an atexit hook” and refers to a “memory worker thread”, butSkillMemorydoesn’t register an atexit hook and it spawns per-call daemon threads instead of running a long-lived worker. This comment should be updated to match the actual behavior.
# Release the memory worker thread. SkillMemory also registers an
# atexit hook, so an aborted run is covered too; this is the clean
# path so a long-lived process does not hold the thread until exit.
docs/reference/config.md:162
- This table row mentions “the executor is retired”, but the implementation doesn’t use an executor; each outbound call runs in a daemon thread and is time-bounded via
Thread.join(timeout). Update the description to avoid implying a thread pool/executor lifecycle that doesn’t exist.
This issue also appears on line 185 of the same file.
| `train.mem0_timeout_seconds` | float | `5.0` | Hard per-call bound. On timeout the executor is retired and training continues; after 3 consecutive failures memory disables itself for the run |
|
Hi @jrauch713-svg — thank you for the careful update. We rechecked We found one remaining Mem0 API-contract issue before merge. The optional dependency currently declares client.search(query, user_id=namespace, limit=limit)That range is incompatible at both ends:
We reproduced the second case locally without making a network request using Mem0 2.0.11, and confirmed the same contract in 2.0.15. It raises Could you please choose and declare one supported Mem0 API generation and add a small contract-level test for its actual search call? The simplest current path would be to target Mem0 2.x, tighten the dependency range accordingly, and call: search(query, filters={"user_id": namespace}, top_k=limit)A live-service test is not required for this fix; a deterministic test against the supported client contract is enough. This is the only merge blocker we found in the latest update, and we would be happy to re-review promptly. We really appreciate the care you have put into addressing the earlier feedback. |
Summary
Adds an optional, opt-in
skillopt/memorybackend that persists skill iterations and reflection summaries to Mem0, and reads a small amount of relevant history back into the Reflect stage so the memory participates in training rather than only recording it.Revised to the reviewable minimum requested in review. The two substantive objections — silent enablement and a write-only design — are both addressed.
What changed since the first version
1. Explicit opt-in. Uploading requires
train.mem0_enabled: true. AMEM0_API_KEYpresent in the environment for another application no longer enables anything; a disabled run does not even read the key. Allmem0_*resolution lives inskillopt/memory/settings.py.2. Redaction before any outbound request.
skillopt/memory/redaction.pyis a single choke point — no payload reaches the client without passing throughredact_for_upload. It strips vendor API keys, bearer/basic tokens, JWTs, private keys,key = valuesecret assignments, the project root, and/home/<user>-style prefixes. Credentials are scrubbed before paths are collapsed, so a key embedded in a home path cannot survive. Relative paths and filenames are deliberately preserved so stored memories stay useful.The patterns mirror
skillopt_sleep/staging.pyrather than importing it, sincepyproject.tomlkeeps that package decoupled with zero research dependency.3. Stable project-specific namespace. Memories are scoped to
skillopt:<env>:<sha256-prefix-of-project-root>— stable across runs of one project, distinct across projects, and the raw path is hashed rather than transmitted. Replaces the previous config-name/env user id that could mix unrelated projects.4. One bounded retrieval call before reflection.
hook_pre_reflectfetches relevant history and appends it to the reflection input, which is what turns the integration from external logging into a memory loop. It is assigned to a separatereflect_contextvariable so the autonomous learning-rate decision and the rewrite prompt continue to see the unaugmented context.Also included
mem0_timeout_seconds(default 5s), so an unreachable service costs one bounded pause instead of blocking a training step.None, strings, non-lists) are filtered rather than raised inside a swallowingtry.mem0optional extra declared, deliberately outsideall.mem0_*keys mapped through_FLATTEN_MAPundertrain.. Without this they were silently dropped for structured configs — which is every config in the repo — leaving the feature inert with no error.configs/features/mem0_memory.yaml, a documented opt-in example following thesoft_gate.yamlconvention.docs/reference/config.mddocuments every key and exactly what leaves the machine.Deliberately not included
Per review guidance: batching, async writes, sophisticated ranking, and a broad benchmark study are left as follow-up work.
Test plan
tests/test_mem0_memory.py— 15 cases, no network. Runs under pytest or directly viapython tests/test_mem0_memory.py.MEM0_API_KEYdoes not enable uploads, and no backend is constructedmem0_namespaceoverride winsmem0_timeout_secondsFull suite: 485 passed, 6 skipped, 0 failed.
ruff checkclean on all new files. Rebased onto currentmain.Note: all tests are mocked — no call has been made against the live Mem0 service.