[YellowSnnowmann]

The bug

build_memory_client() in linkedin_enrichment.rs used MemoryClient::new_local():

pub fn new_local() -> Result<Self, String> {
    let workspace_dir = crate::default_openhuman_dir()?.join("workspace");
    Self::from_workspace_dir(workspace_dir)
}
// default_openhuman_dir() == dirs::home_dir()?.join(".openhuman")

It resolves ~/.openhuman/workspace from the home directory and never consults OPENHUMAN_WORKSPACE. On any host that scopes its workspace — every hosted tenant, and any local run with the variable set — the scraped LinkedIn profile was written to a store nothing else reads.

It failed by succeeding. The write returned Ok; the profile simply was not there afterwards. That is why nothing surfaced it.

The fix

Take the process-wide client, which is the workspace the host actually bound.

The caller already handles the error path — it warns and skips persistence — so a host that has not bound memory yet now loses the write rather than misplacing it. That is the better of the two failures: one is visible in logs, the other is invisible until someone asks why the agent has no profile context.

On the allowlist entry

new_local was also a second engine construction against the same data directory — one call deeper than MemoryClient::from_workspace_dir, which bypass_allowlist_tests already refuses because it "risks a second ingestion worker on one store". The scanner had no needle for new_local, so that hazard was invisible.

The allowlist gains an entry; the bypass count does not change. This site always bypassed the guard. It did so through a pattern nothing scanned for, and now does so through one that is scanned, declared, and reasoned. The list is more accurate after this change, not more permissive.

Routing it through the guard instead is not available. It needs MemoryClient::store_skill_sync, which derives the skill-<id> namespace and keys the upsert on an opaque document_id precisely so a scraped profile title never reaches the secret/PII key guard — the method's own comment explains that a title "can legitimately contain verification codes, token-rotation notices, or other secret-/PII-looking strings". No provider family carries that shape, and reimplementing it at the call site is the duplication the method exists to prevent.

docs/specs/memory-guard-allowlist.md gains the matching row in section D.

One thing the lints caught me on

An earlier revision added MemoryClient::new_local( to BYPASS_PATTERNS. bypass_patterns_are_all_live rejected it:

these BYPASS_PATTERNS needles match nothing in the tree: ["MemoryClient::new_local("]
… A needle that matches nothing is a guard that guards nothing.

Correct — after the fix nothing calls it. Removed. no_new_memory_driver_bypasses then flagged the global::client( site, which is what produced the declared entry above rather than a silent swap of one bypass for another.

Validation

Command	Result
cargo check -p openhuman --lib	clean
cargo clippy -p openhuman --lib	clean
cargo fmt --all -- --check	clean
cargo test -p openhuman --lib -- bypass_allowlist direct_engine_refs	12 passed, 0 failed
cargo test -p openhuman --lib -- linkedin	9 passed, 0 failed

Scope

Three files: the fix, the allowlist entry, the spec row. No feature flags, no dependency changes.

Found while working tinyhumansai/tinymemory#18 §C4 (removing the OpenHuman framing from tinymemory-core), but it stands on its own — the workspace bug is real today, independent of that work.

Summary by CodeRabbit

• Bug Fixes

  • LinkedIn enrichment now saves profile information through the configured shared memory workspace, improving consistency and reliability.
  • Existing behavior is preserved when memory services are unavailable: enrichment skips persistence and provides a warning instead of interrupting processing.

• Documentation

  • Documented the LinkedIn enrichment memory access pattern and its approved exception to memory access safeguards.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0778889c-124b-474e-a34d-286a2fa050cf

📥 Commits

Reviewing files that changed from the base of the PR and between 07e08a8 and 78c0bac.

📒 Files selected for processing (3)

• docs/specs/memory-guard-allowlist.md
• src/openhuman/agent/learning/linkedin_enrichment.rs
• src/openhuman/memory/bypass_allowlist_tests.rs

Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

LinkedIn enrichment now asynchronously obtains the configured active memory client. The bypass allowlist test and specification document record the call site and its persistence behavior.

Changes

LinkedIn memory client

Layer / File(s)	Summary
Active memory client integration and allowlist src/openhuman/agent/learning/linkedin_enrichment.rs, src/openhuman/memory/bypass_allowlist_tests.rs, docs/specs/memory-guard-allowlist.md	build_memory_client now awaits active_memory_client() and returns a MemoryClientRef. Existing error mapping and skip behavior remain. The allowlist test and specification document record the bypass and persistence details.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🔵 Low · up to 78c0b

This change makes LinkedIn profile persistence use the host-bound workspace instead of an unintended home-directory store. A bounded security risk remains if multiple workspaces share one process or the global binding is stale, because profile data could be saved to the wrong tenant; merge is reasonable with explicit owner awareness and follow-up on workspace-context isolation.

Suggested reviewers: senamakel

Poem

A rabbit hops as memories flow,
The active client now says hello.
LinkedIn skills find their workspace place,
Allowlist notes record the trace.
“Nibble-safe!” the bunny beams.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: LinkedIn profile persistence now uses the host-bound workspace.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[tinysweeper]

tinysweeper found nothing blocking. Approving.

$0.0000 · 0 in / 0 out · 242 embedded · openrouter/openai/text-embedding-3-small

[YellowSnnowmann]

@coderabbitai review

The earlier run reported "Review failed — an error occurred during the review process" rather than any findings, so this PR has not actually been reviewed by CodeRabbit. Requesting a fresh pass.

For context while it runs: the change is three files — a one-line swap of MemoryClient::new_local() for tinymemory_core::global::client() in linkedin_enrichment.rs, a matching ALLOWED entry in bypass_allowlist_tests.rs, and the corresponding row in docs/specs/memory-guard-allowlist.md.

Separately, the failing Markdown Link Check is not from this PR. All four reported 404s are in docs/specs/plan-agents.md and docs/superpowers/specs/2026-07-24-identity-marketplace-seller-web-only-design.md, neither of which this PR touches — my diff is the three files above. The links are also not actually dead: tinyhumansai/tinyagents is public, issues 88 and 89 exist, docs/spec/host-capability-traits-rfc.md exists, and openhuman#4920 exists. The same job logged 429 Too Many Requests and 503 while downloading its own action, so the runner was being rate-limited by GitHub at the time. I have re-run it.

[coderabbitai]

@YellowSnnowmann I will run a fresh review for #5575. The review will assess the current diff independently. The reported Markdown Link Check failure is outside the stated PR scope.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[tinysweeper]

How this change flows

2 changed behaviours across 1 relationship. The code graph does not know these behaviours yet — normal for newly added code, and a cold index otherwise. 2 further behaviours left out to keep the diagram readable.

flowchart LR
  n0["run_linkedin_enrichment<br/>changed"]:::changed
  n1["scrape_linkedin_profile<br/>changed"]:::changed
  n0 -->|calls| n1
  classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
  classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
  classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
  classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff

Loading

_{Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge.}

[CodeGhost21]

The fix is right. The justification attached to it is not — one clause of the allowlist reason describes a protection this call site specifically does not get.

Verified against the pinned submodule (vendor/tinymemory @ dc3a725), not just the diff.

The fix holds. global::client() returns the workspace init(…) bound (core/src/global.rs:170), with no ~/.openhuman fallback — that absence is documented there as deliberate. MemoryClientRef = Arc<MemoryClient>, so persist_linkedin_profile(&MemoryClient, …) still resolves by deref. Allowlist entry is sorted correctly (path, then pattern) and the spec row landed in the right section (D).

---

1. Blocking — the allowlist reason claims a protection this site doesn't receive

Both the ALLOWED entry (bypass_allowlist_tests.rs:193) and the spec row (memory-guard-allowlist.md:165) say store_skill_sync:

keys the upsert on an opaque document_id so a scraped title never reaches the secret/PII key guard

But this caller passes None for document_id — linkedin_enrichment.rs:723, and again at :745 for the URL-only fallback. And store_skill_sync names this exact caller as the exception to that behaviour:

// Callers without a stable id (e.g. LinkedIn enrichment) keep the
// title as the key, unchanged.

— vendor/tinymemory/core/src/store/client.rs:305

So the upsert key here is the title, LinkedIn profile: {url}, and it does go through upsert_document's secret/PII key guard. Not a practical bug — a LinkedIn URL won't trip the detector — but this list's own doc says the reason string is "what a future reviewer relies on to tell 'deliberate' from 'forgotten'", and as written it states the opposite of what the code does. That's the one thing an allowlist entry cannot afford to get wrong.

The entry still looks justified, just on different grounds: store_skill_sync derives the skill-<id> namespace and stamps every write MemoryTaint::ExternalSync (client.rs:333-336) so the subconscious gate can see the provenance — neither of which any guard-routed provider family exposes. Suggest rewriting both the entry and the doc row around those two, and dropping the document_id clause.

2. Consider — global::client() vs ops::helpers::active_memory_client()

The PR frames "a host that hasn't bound memory loses the write" as the unavoidable half of the trade. It isn't quite. memory::ops::helpers::active_memory_client() (helpers.rs:379) returns the same MemoryClientRef and falls back to global::init(Config::load_or_init().workspace_dir) when startup wiring hasn't run — explicitly not to ~/.openhuman, so it doesn't reintroduce the bug this PR fixes.

Two things that would buy:

• No lost write in the pre-init case. The skip-and-warn branch stays for genuine failures.
• It resolves the same workspace write_profile_md(config, …) writes PROFILE.md into. As written, the two halves of run_linkedin_enrichment derive their target workspace from different sources — config for the file, the global binding for memory — and can in principle disagree.

Cost is identical either way: active_memory_client( is itself a BYPASS_PATTERNS needle (bypass_allowlist_tests.rs:81), so the allowlist gains exactly one entry regardless of which accessor you pick.

If global::client() is the deliberate choice — "nothing bound memory ⇒ that's a wiring bug, drop the write loudly rather than auto-binding" — that's a defensible position, but a stronger claim than the current doc comment makes. Worth stating outright so the next reader doesn't 'fix' it. Both entry points reach here via config_rpc::load_config_with_timeout() inside a running daemon (tools.rs:604, schemas.rs:593), so init has almost certainly run; this is about which failure you want in the case where it hasn't.

3. Follow-up, not for this PR

MemoryClient::new_local() is now referenced by nothing in either tree — grep finds only its definition at vendor/tinymemory/core/src/store/client.rs:138 plus the prose in this PR. It's the footgun that produced this bug, and no lint on the openhuman side can hold the line on it: a BYPASS_PATTERNS needle for it would fail bypass_patterns_are_all_live, exactly as you hit. Deleting it or #[cfg(test)]-gating it in tinymemory alongside #18 is what actually closes the hazard; otherwise the next caller reintroduces the same silent misplacement.

---

Only #1 blocks. CI is green across the board — Rust Quality, Core Coverage, Feature-Gate Smoke all pass.

[YellowSnnowmann]

@CodeGhost21 — both taken, in 78c0bac. Thank you for checking this against the pinned submodule rather than the diff; #1 is a real defect and I would not have caught it from the PR alone.

1. Blocking — the reason string overclaimed

Confirmed, and worse than a wording slip: it stated the opposite of what the code does. Both call sites pass None (linkedin_enrichment.rs:723, :745), and store_skill_sync names this caller as the exception in its own comment —

// Callers without a stable id (e.g. LinkedIn enrichment) keep the title as the key, unchanged.

— so the key is LinkedIn profile: {url} and it does go through upsert_document's guard.

Rewritten in both places on the two grounds that actually hold: store_skill_sync derives the skill-<id> namespace and stamps every write MemoryTaint::ExternalSync (client.rs:333-336) so the subconscious gate sees the provenance through the persistence layer. Neither is reachable through a guard-routed provider family.

One deliberate difference from your suggestion: rather than just dropping the document_id clause, the entry and the doc row now say outright that this site does not get that protection. A silent removal leaves the next reader unable to tell whether it was considered and rejected or simply forgotten — which is the same failure mode as the overclaim, one step quieter.

2. Consider — switched to active_memory_client()

Took it. I verified each leg before changing:

• It falls back to global::init(current_workspace_dir()), and current_workspace_dir is Config::load_or_init().workspace_dir (helpers.rs:363) — so it cannot reintroduce the ~/.openhuman pin, as its own doc states.
• write_profile_md writes to config.workspace_dir (linkedin_enrichment.rs:372). So the divergence you describe is real, not theoretical: the two halves were deriving the target from different sources.
• active_memory_client( is already at bypass_allowlist_tests.rs:81, so the allowlist gains one entry either way — the allowlist pattern for this file changed from global::client( to active_memory_client(, not gained a second row.

build_memory_client is async now; the skip-and-warn branch stays for genuine failures. The doc comment states the reasoning rather than leaving "which accessor" to be re-derived.

3. Follow-up — agreed, and it belongs in tinymemory

new_local() has no callers left in either tree. You are right that no openhuman-side lint can hold that line: a BYPASS_PATTERNS needle for it fails bypass_patterns_are_all_live, which is exactly what I hit.

That work is live right now — tinymemory#18 §A3 landed as #42, which adds create_memory_provider, the first constructor that binds this crate's own store through MemoryProvider. new_local is the obvious next thing to #[cfg(test)]-gate or delete there, and I will raise it against #18 rather than leaving it implicit.

Validation

cargo fmt --check                                          clean
cargo clippy --lib                                         clean
cargo test --lib openhuman::memory::bypass_allowlist       6 passed
cargo test --lib linkedin                                  9 passed

bypass_allowlist_has_no_stale_entries is the one that matters for the pattern change — it would fail if active_memory_client( did not actually match at that path.

[CodeGhost21]

Both points taken, and taken correctly. Re-verified 78c0bac1 against the pinned submodule (vendor/tinymemory @ dc3a725, unchanged by this PR) rather than the diff. Approving.

1 — the reason string now matches the code

Both the ALLOWED entry and the section-D row now rest on the two grounds that hold, and I confirmed each at the source:

• store_skill_sync derives format!("skill-{}", skill_id.trim()) (client.rs:305) and stamps taint: crate::MemoryTaint::ExternalSync on every NamespaceDocumentInput it builds (client.rs:333-336).
• The None path keys on the title — let key = match stable_id { Some(id) => …, None => title.to_string() } — and both call sites still pass None (linkedin_enrichment.rs:739, :761).

Stating outright that this site does not get the opaque-document_id protection, rather than dropping the clause, is the better call and I'd have been wrong to suggest otherwise — a silent removal leaves the next reader unable to distinguish "considered and rejected" from "forgotten", which is the same failure one step quieter.

2 — active_memory_client(), and it does not reintroduce the hazard

Checked the thing worth checking about auto-init, since a fallback that constructs is a different risk from one that reads: global::init is not a one-shot OnceLock::set. It returns the existing handle for the same workspace, and consults WORKSPACE_CLIENTS before constructing — the comment there names this exact hazard ("building a second client for A would put two ingestion workers over A's SQLite file"). cache_client resolves the construct-concurrently race by letting the first insert win. So the pre-init path yields one engine per workspace, not a second one over the store, and a later login rebind still replaces cleanly.

Also confirmed:

• current_workspace_dir() is Config::load_or_init().workspace_dir (helpers.rs:363) — no ~/.openhuman fallback anywhere on the path.
• MemoryClientRef = Arc<MemoryClient> (client.rs:28), so persist_linkedin_profile(&MemoryClient, …) still resolves by deref from &MemoryClientRef.
• The allowlist pattern for this path changed from global::client( to active_memory_client( rather than gaining a second row; active_memory_client( was already a BYPASS_PATTERNS needle (bypass_allowlist_tests.rs:81), and the entry is well-formed under bypass_allowlist_entries_are_well_formed and correctly sorted. Bypass count unchanged.

CI is green — Rust Quality, Feature-Gate Smoke, Markdown Link Check all pass on this head; only Core Coverage is still running.

---

Two non-blocking notes, neither for this PR:

• The section-D row quotes the key as LinkedIn profile: {url}, which is the full-scrape title. The fallback writes LinkedIn profile URL: {url} (:753). Both go through the guard identically, so the reasoning is unaffected — "the title" would just be exact where "LinkedIn profile: {url}" is one of two.
• Consequence of that same title-keying, pre-existing and untouched here: a URL-only fallback followed by a later successful scrape leaves two documents in skill-user-profile, since the two titles are different keys. If that matters, a stable linkedin:{url} document_id at both sites would collapse them — but that changes dedup behaviour and belongs in its own change, not this fix.

And agreed on #3 — new_local() is the footgun, no openhuman-side lint can hold that line, and gating it in tinymemory alongside #18 is what closes it.