Skip to content

feat(skills): import-claude-context — bring a person's Claude Code history into their Sutando, on consent, with a review gate - #4127

Merged
bassilkhilo-ag2 merged 19 commits into
mainfrom
feat/import-claude-context
Sep 11, 2026
Merged

bassilkhilo-ag2 merged 19 commits into
mainfrom
feat/import-claude-context

Conversation

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

Why

Most people arrive with history in Claude Code and none of it reaches their Sutando (Vivek's example: ask Claude whether you met a company and it knows; ask Sutando and it cannot help). This in-box skill closes that gap once, with two consents, without touching the stock ~/.claude.

What

skills/import-claude-context/ — index.py (LLM-free index; --counts-only is readdir+stat in ms), extract.py (session-recap's extract with a new --root, noise-stripped, scan_and_redact, ≤120k-char dumps 0600), finalize.py (stages everything under data/claude-import/staged/ with a review.md digest; --commit [--projects] writes the ≤2 KB memory file + budget-guarded MEMORY.md row + notes/claude-import/*.md + People payloads; --discard; deterministic people/company merge), haiku prompts, and a SKILL.md procedure: index → immediate ack to the owner DM → extract → one background coordinator subagent (summaries → roll-ups → entities → stage → digest) → [no-send]; commit only on the owner's "bring it in". Never writes under ~/.claude; skips subagent transcripts; redacts before disk; counts-only logs; nothing leaves the Mac without --cloud.

Proven on the owner's real history (2026-09-10)

47 sessions / 4 projects / 603 MB on disk → index + extract 15 s, 19 secrets redacted, find ~/.claude -newer marker = 0; 42 haiku session summaries → 4 roll-ups → entities (14 people after merge, 43 companies, 130 decisions, 30 open threads); staged digest reviewed by the owner. Memory summary 982 bytes.

Tests

import-claude-context-{index 13, extract 9, finalize 12, stage 21}; session-recap-extract 13 and context-resume 17 unchanged; lint-claude-home-path clean; ruff clean.

Pairs with

ag2space-cinny-desktop (claude_import_scan/queue/status) and cinny-webclient (onboarding card + Settings → Claude Code history).

🤖 Generated with Claude Code

https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY

bassilkhilo-ag2 and others added 3 commits September 10, 2026 17:27
…story into their Sutando, read-only and on consent

Most people arrive with history in Claude Code, and none of it reaches
their Sutando: ask Claude whether you met a company and it knows; ask
Sutando and it cannot help. This in-box skill closes that gap once, with
consent, without touching the stock Claude tree.

What it reads: `~/.claude/projects/<slug>/*.jsonl` via
`claude_home_path("projects", vanilla=True)` — the STOCK home, never the
core's relocated `.claude-sutando`. Top-level transcripts only;
`subagents/**` and sidechain sessions are skipped. Files are opened
read-only; an out-dir inside the root is refused; the tests assert the
root's listing and mtimes are unchanged after a run.

Three scripts, all under `skills/import-claude-context/`:
  index.py     LLM-free, one streaming pass (JSON parsed only for the short
               meta lines): last ai-title, last prompt, agent name, first /
               last timestamp, cwd, message and tool-line counts, dialog
               bytes. `--counts-only` is readdir + stat only (~80 ms on the
               owner's 603 MB tree) — what the onboarding card shows.
               Writes index.json + a checklist index.md, state.json,
               status.json; `--dry-run` writes nothing; `--json` prints
               counts only.
  extract.py   no parsing of its own: session-recap's extract with a new
               `--root`, then context_resume's noise regexes, then
               secret_scanner.scan_and_redact, then ≤120k-char dumps split
               only at turn boundaries, 0600 in a 0700 dir. `--new` re-runs
               touch only sessions whose (mtime, size) changed.
  finalize.py  summaries → sinks: `memory/claude_import.md` (≤ 2,000 B) and
               the MEMORY.md row only when memory-index-budget.py allows it;
               `notes/claude-import/<slug>.md` (+ overview, dated Update
               sections on re-runs); ≤ 25 People payloads with ≥ 2 citations
               (`source: claude-import`); `--forget <slug>`; `--purge-dumps`.
Summarisation is the session-recap pattern: haiku subagents, map-reduce over
dump files they read and write themselves; prompts in prompts/*.md with the
JSON schemas spelled out. The cloud upload stays behind a separate,
explicit `--cloud` and never runs in onboarding.

Smallest edits elsewhere: context_resume.py exposes message_text /
clean_text / the noise regexes; session-recap's extract.py gains `--root`
(its tests unchanged). docs/built-in-tools.md and CHANGELOG-PENDING.md note
the skill.

Tests: import-claude-context-index 13, -extract 9, -finalize 12 (fixture
tree with sidechain, subagents, repeated titles, cwd recovery, counts-only
opens nothing, dry-run writes nothing, out-dir-inside-root refused, --new,
root untouched; noise stripped and keys redacted, chunking at turn lines,
0600; memory cap, guarded MEMORY.md row, notes layout, people cap,
--forget). session-recap-extract 13 and context-resume 17 still green;
lint-claude-home-path clean; ruff clean.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
…roves before anything lands

Owner rule (2026-09-10): "we should be able to see the context and then
approve to bring it in." Until now finalize wrote memory, notes and People
payloads straight after summarising. It now stages by default and commits
only on an explicit yes.

finalize.py
  --stage (default)   renders everything under data/claude-import/staged/:
                      memory/claude_import.md, notes/claude-import/*.md,
                      people.json, manifest.json (a content fingerprint of
                      index + summaries + roll-ups + entities) and review.md —
                      the digest for the owner: per project a paragraph, open
                      threads, decisions, the people it would add with
                      citation counts, the memory size, and the reply
                      options. Touches no sink; status.json phase `staged`.
  --commit [--projects] moves the staged set into the real sinks with the
                      existing guards (≤2,000 B memory file, budget-guarded
                      MEMORY.md row, dated Update sections); sets
                      summarized_at and phase `done`; refuses when nothing is
                      staged, when the staged set is stale (fingerprint), or
                      when a selector is ambiguous; a partial commit re-stages
                      the rest and re-renders the review.
  --discard [--projects] drops staged output without touching the sinks.
  merge_people / merge_companies (pure): one person per shared email; a bare
                      first name folds into the unique fuller name that starts
                      with it when no email conflicts; never two multi-token
                      names; companies by case-insensitive name. On the
                      owner's real history: 16 → 14 people (Michael/Michael
                      Silton by email, Qingyun/Qingyun Wu, Chi/Chi Wang,
                      Alejandro/Alejandro Guerrero).

SKILL.md — the procedure is now three tasks: the main task indexes, writes
an immediate acknowledgement to the owner DM ("Importing your Claude Code
history: N conversations across M projects. I'll message you here when it's
ready to review…"), extracts, spawns ONE background coordinator subagent
(CLAUDE.md: "in the background means spawn a subagent") and returns
[no-send] so the task loop stays free; the coordinator runs the haiku
summaries, roll-ups, entities and --stage, then posts the digest to the
owner DM (or a failure note with the resume hint); a later task turns
"bring it in" / "bring in <slug>" / "forget <slug>" into --commit /
--commit --projects / --discard. New never-do lines: never commit without
the owner's explicit yes in the same conversation; never block the task
loop on summarisation; never post the digest outside the owner DM.

Tests: new import-claude-context-stage 20 (stage leaves sinks untouched,
review.md contents and footer, commit guards and phase, partial commit,
discard, stale and empty refusals, ambiguous selector, the merge cases,
people_merged end to end); finalize 12, index 13, extract 9 unchanged;
ruff clean.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
…tifier, not a name

The summariser sometimes writes "Cyrus (cyrus@x.com)" or "Rui <rui@ag2.ai>" as
a person's name. _norm_name drops the parenthesised/bracketed email,
_emails_of keeps it, and singleton entries are cleaned too (_clean_entry),
so the display name is clean, the address is not lost, and the email-group
merge still unifies the entry with a bare "Cyrus" carrying the same
address. Seen on the owner's real history 2026-09-10; pinned by a stage test.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • skills/import-claude-context/scripts/_common.py (100%)
  • skills/import-claude-context/scripts/extract.py (100%)
  • skills/import-claude-context/scripts/finalize.py (100%)
  • skills/import-claude-context/scripts/index.py (100%)
  • skills/import-claude-context/scripts/progress.py (100%)
  • skills/session-recap/scripts/extract.py (100%)
  • src/context_resume.py (100%)

Summary

  • Total: 2085 lines
  • Missing: 0 lines
  • Coverage: 100%

@yixuan-ag2 yixuan-ag2 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.

Reviewed at 6e68b595. The shape of this is right — read-only on the source, staged behind an explicit "bring it in", 0600 dumps in a 0700 dir, and a test that asserts the secret material is gone (assertNotIn(GHP), assertNotIn(AIZA)) rather than just counting redactions. One blocker, and it is one line to fix.

Blocker — an Anthropic API key is not redacted, in the one corpus where it is the likeliest secret

redact() is scan_and_redact plus _EXTRA_SECRET_PATTERNS, and neither covers sk-ant-. _PLUGINS_CONFIG in src/secret_scanner.py has 19 detectors and no Anthropic one; _EXTRA_SECRET_PATTERNS has exactly one entry, Google API Key.

Run against your redact(), with extract.py, _common.py, index.py, context_resume.py, secret_scanner.py and util_paths.py all fetched at 6e68b595:

fixture                                        n   verdict
OpenAI (built from OpenAIDetector's own regex)  1   redacted     <- control
GitHub  ghp_…                                   1   redacted     <- control
Google  AIza…                                   1   redacted     <- control
Anthropic  sk-ant-api03-…                       0   SURVIVES
  …same key inside a USER turn                  0   SURVIVES
  …same key as ANTHROPIC_API_KEY="…"            1   SURVIVES

Three controls redact, so the zeros are absence and not a broken harness. Two checks against my own setup being the cause: DETECT_SECRETS_ACTIVE is True here (the module degrades silently when detect_secrets is missing, which would have faked exactly this result), and src/secret_scanner.py is byte-identical between my checkout and your head.

Worth saying plainly why this one matters more than a generic "add more patterns": this skill's whole input is Claude Code transcripts. An Anthropic key is the single most probable secret in that corpus, and the dumps do not stay put — extract.py's own docstring says they are "chunked for a cheap summariser", so the text goes to a model, and --cloud sends it off the Mac. Redaction is the boundary, and this is the shape that walks through it.

You already built the mechanism for precisely this case: _EXTRA_SECRET_PATTERNS exists because scan_and_redact misses AIza. This is the same gap with a likelier key, so the fix is one more entry plus a fixture alongside GHP/AIZA in test_noise_stripped_and_secrets_redacted. Please confirm the current key format against Anthropic's docs rather than taking a regex from me.

Note for whoever adds that pattern — the count is not a removal proxy

The quoted row above returned n=1 while the full key survived verbatim: something (entropy plugin) fired on part of the line without replacing the key. So redactions >= 1 can be true of text that still contains the secret.

Your test does not fall for this — it pairs the count with assertNotIn — and that pairing is the thing to keep. A new shape should get its own assertNotIn, never just bump the count.

Cross-repo, since no CI can see it

ag2-space/cinny-webclient#958 is the UI half of this same feature and was opened minutes apart. I'm reviewing them as a pair; anything about consent wording or the "bring it in" contract should stay consistent across both, and it's worth naming the intended merge order in one of the two descriptions.

Everything else I looked at held up. Happy to re-review as soon as the pattern lands.

…st; never install

prompts/session.md never reached the repo: the root .gitignore's SESSION.md
rule matches it case-insensitively on macOS, so the shipped skill had no
per-session prompt and every install had to reverse-engineer the summary
schema from project.md/entities.md (found on the first fresh-install run,
2026-09-10). Renamed to prompts/session-summary.md and pinned by a test that
every prompt SKILL.md names exists and is not ignored.

SKILL.md: answer the owner's first-contact greeting before starting the
import; never pip-install detect-secrets (or anything) to run the skill —
DEGRADED scanner mode proceeds with the built-in rules and gets one line in
the digest, not an offer in the DM.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
@yixuan-ag2

Copy link
Copy Markdown
Collaborator

Second finding, separate from the sk-ant- one and cheaper to fix. I raised it in full on the UI half (ag2-space/cinny-webclient#958); this is the part that lands here.

SKILL.md asserts both sides of a contradiction. The description: field says "Read-only on ~/.claude; conversation text only; nothing leaves the Mac." The overview four lines later says "cheap-model subagents summarise it", and step 5 is explicit: one Agent-tool subagent per session with model: haiku, reading the dump chunks. Plus a haiku roll-up per project and a haiku pass over all summaries for entities.json.

Those are Anthropic API calls carrying transcript-derived text. Unless there's a local inference path I've missed, "nothing leaves the Mac" is not what the skill does. (finalize.py --people-json → the station's people__upsert_person is a second path, though post-commit and behind its own yes, so much less pointed.)

This matters more than a docs nit because #958 renders that promise as consent copy the user reads before deciding: "Read-only and stays on your Mac. Nothing is uploaded." Two PRs, same claim, and the UI is where it becomes a representation to the user.

In fairness: the corpus is Claude Code transcripts, so this content already transited Anthropic once. A reasonable person could argue haiku summarisation exposes nothing new, and I don't think the wording was meant to overstate. But it does compound with the redaction gap above — an unredacted sk-ant- key is re-transmitted, in the clear, under a sentence promising nothing is sent.

The elsewhere-in-this-file precision is what makes the line stand out: "conversation text only (no tool I/O)", "read-only on ~/.claude", the vanilla=True note about never touching the relocated .claude-sutando. Those are exact. This one is the outlier, and I'd bet it predates the haiku design rather than contradicting it deliberately.

Suggest saying what happens: reading is local, summarising is done by Claude on the owner's behalf, and nothing goes to a third party. Whatever wording you land on, #958's panel needs to match it.

…r `conversations`; progress.py keeps status.json true

The fresh-install run of 2026-09-10 printed `"extracted": 47, "errors": 0`
while 43 dump files existed. Four sessions had no assistant turn at all
(aborted / never answered: user_msgs 2/3/0, assistant_msgs 0): their cleaned
dialog produced no chunk, write_chunks wrote nothing, and the loop counted
them anyway. A fifth survived at 178 bytes (median 86,630), passed the
emptiness test and produced a useless haiku summary. status.json later said
`sessions 43, skipped_empty 4` — the truth existed one stage late — and the
acknowledgement (SKILL.md step 2) was built from the index count, so the
owner was told "47 conversations" while the digest describes 43.

extract.py
- A session whose cleaned dialog yields no chunk, or fewer than --min-chars
  characters of it (default 400), is not extracted: no dump (an older dump
  of the same session is removed), `skipped_empty` in the counts and in
  status.json, and `skipped_empty: true` + `extracted_at`/mtime/size in its
  state.json record so a --new re-run leaves it alone until the file
  changes. `extracted` now counts only sessions that wrote >= 1 chunk; the
  human summary line names the skipped count.

index.py
- Per-project and top-level `empty` (sessions with assistant_msgs == 0) and
  `conversations` = sessions − empty, in --json, status.json, the human line
  and the index.md header. --counts-only is readdir+stat and cannot know:
  both are null there and the docstring/SKILL.md now call it a file count.
  The index.md instruction to tick `[x]` by hand is gone (progress.py counts).

finalize.py
- session_total (and the new project_sessions used by the review, overview
  and note headers) subtract the skipped_empty sessions read from state.json,
  so the digest, the memory file and the overview say 43, not 47. A skipped
  session has no summary, is never pending and never blocks --commit.

progress.py (new)
- The coordinator was not keeping status.json current: on the live VM run
  it still said `summarized 0` (updated 14:40:50) while 38 of 43 summaries
  existed. progress.py recounts from disk with no model — index sessions
  minus skipped_empty, merged summaries present (partials not), roll-ups,
  entities.json, staged/manifest.json — and rewrites status.json through
  write_status with phase summarizing | rolling-up | staged. Counts only.
  SKILL.md steps 5–7 now run it after each batch of summaries, after the
  roll-ups and after entities instead of hand-editing status.json.

SKILL.md
- Step 2 uses `conversations` from the index and appends "(K empty ones
  skipped)" only when K > 0; step 1 and the consent line say --counts-only is
  a file count and `conversations` is the number the owner is told; step 3
  documents the skip rule; CLI table gains --min-chars and progress.py.

Tests: extract (empty and 100-char sessions skipped and counted, a normal
one extracted, --new leaves them alone, --min-chars 0 keeps the tiny one,
a session that turns empty loses its dumps), index (`empty`/`conversations`,
counts-only nulls, human line), stage (a skipped_empty session is neither
counted nor pending and --commit lands), new progress test (phase ladder,
partials not counted, counts-only output, no index refused). All
import-claude-context-*.test.py and session-recap-extract.test.py pass.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
@yixuan-ag2

Copy link
Copy Markdown
Collaborator

Re-checked at 4775fb9e since the head moved — I said I'd be prompt, and I don't want the standing request-changes to read as stale bookkeeping.

The redaction blocker is unchanged. Same test, same controls, against the new head's redact() with every dependency at 4775fb9e:

fixture                      n   verdict
Anthropic sk-ant-…           0   SURVIVES
  …same key in a USER turn   0   SURVIVES
Google AIza…  CONTROL        1   redacted
GitHub ghp_…  CONTROL        1   redacted

_EXTRA_SECRET_PATTERNS is still ['Google API Key']. Both commits in the range are about the prompt file and the counting/progress work, so I don't read this as a fix that missed — just as not-yet.

One interaction worth flagging, because the new degraded-mode handling touches exactly this. The added rule says that when secret_scanner reports mode: DEGRADED, the run proceeds on the repo-local rules and the digest gains:

Note: secret redaction used the built-in rules only on this Mac.

Good call on proceeding rather than installing anything, and on telling the owner. But that sentence points at _EXTRA_SECRET_PATTERNS — the set that is missing sk-ant-. So on a degraded Mac the owner is reassured about the rule set with the gap, and on a healthy one detect-secrets has no Anthropic plugin either (19 configured detectors, none for it). Neither path covers the key, which is why I'd still like the one-line pattern before this lands.

Nothing needed on my SKILL.md comment above — I posted it about ten minutes after your second push, so it wasn't something you could have picked up in this range.

The rest of the range looks like an improvement to me. conversations = sessions − empty being the number the owner is told is more honest than a file count, and progress.py recounting from disk rather than anyone writing status.json by hand is the right call — hand-maintained status is exactly what drifts.

@marklysze marklysze 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.

Checked the 3 failing checks directly (additive to yixuan-ag2's engineering-shape review — this is just the CI specifics).

typecheck + node & shell tests fails on two separate review-checks.sh gates:

  • hardcoded-paths (2): skills/import-claude-context/scripts/index.py:2 and skills/session-recap/scripts/extract.py:7 both spell ~/.claude/projects literally in a docstring. Route through the workspace/config helper this repo uses for that path, or add a scoped REVIEW.md allow if the docstring itself is the reason it needs to be literal.
  • prose-cap (10 blocks over the 2-line cap): extract.py:54/68/262, finalize.py:533, index.py:55/65 (9 lines), context_resume.py:53/88, import-claude-context-extract.test.py:36, import-claude-context-stage.test.py:482.

diff coverage >= 95% (python): 94% (81/1493 changed lines uncovered). Concentrated in finalize.py (94.1%, the largest gap by far — commit/stage/discard error paths mostly) and _common.py (88.4%); extract.py/index.py are close (96%+) but still short.

tsc + tests (clean install) is the aggregator; red because of the typecheck job above.

No new design-level findings from me — the shape yixuan already reviewed is what's here. This is the mechanical list to clear before the diff is worth a fresh content pass.

…lash summaries; haiku subagents become the fallback

SKILL.md steps 5–7 used to fan out one Claude Code haiku subagent per session
(plus one per project and one for the entities pass). On a fresh install the
owner's 43-session import took more than ten minutes in that stage — most of it
agent start-up and Read/Write round-trips, not model time — and the desktop's
progress card sat on it the whole while.

The model work is now ONE stdlib script, `scripts/summarize.py`, that calls
Gemini directly (`generativelanguage.googleapis.com/v1beta/models/<model>:
generateContent`, `x-goog-api-key` header, default `gemini-2.5-flash`; the
-lite model 404s for at least one live key so it is not the default) with a
schema-enforced JSON reply (`responseMimeType: application/json` +
`responseSchema`, temperature 0.2) and 16 requests in flight. The credential
comes from `credential_resolver.resolve_credential("gemini-text")` — the
platform-managed key first, then the BYO `GEMINI_API_KEY` chain — and is sent
in the header only, never printed, logged or written. When nothing resolves the
script prints `{"gemini": "unavailable"}` and exits 3, and SKILL.md keeps the
whole haiku-subagent procedure as the fallback block (step 6).

Measured on the owner's real, already-extracted and redacted dumps (43
sessions, 76 chunk files, 5.6 MB) at concurrency 16: 3 min 41 s end to end —
sessions 141 s (43 requests), roll-ups 81 s and entities 76 s running side by
side (both read only the summaries); 70 requests, 2.13M prompt / 0.44M reply /
0.05M thinking tokens, zero errors, zero 429s. `finalize.py --stage` on the
result renders 4 notes + overview, a 1,038-byte memory file and 25 People
payloads (the haiku run staged 14: the Gemini entities pass is more exhaustive,
the review gate is what trims it).

What it took to get there — three things the naive "one request per unit"
design got wrong on the live data, each measured before being changed:

- The gtm project roll-up (39 sessions, 191k prompt tokens) never finished:
  Flash with thinking off enumerated `open_threads` / `key_decisions` until the
  65k output cap (45 KB of half-duplicate items at ~128 tok/s), which is
  longer than the 240 s request timeout, so every retry timed out too — a
  20-minute stall. Roll-ups now cap the reply at 16k tokens (a MAX_TOKENS reply
  is an error, never a half-written file), split a project whose summaries
  exceed `--group-max-chars` (200k) into parallel parts + ONE merge request in
  the same schema, and think with a 2048-token budget: a 9-session part
  without thinking ignored the prompt's list caps (55 open threads, 73 PR
  refs, 8k tokens; the 12-session parts overran 16k and were cut); with the
  budget it kept 15/15/20 at 5k tokens for the same wall time. prompts/
  project.md gains the list-cap rule so the haiku path benefits too.
- The entities reply grows with the sessions in the request: 4 gtm sessions
  gave 19k output tokens in 79 s, 9 or 12 sessions were cut at 32k after
  130–260 s on every attempt. The pass now runs in project-aligned groups of
  ≤ `--entities-group-chars` (50k, 3–4 sessions) in parallel, and the groups
  (plus the previous entities.json on a re-run) are merged in CODE —
  finalize.py's people/company merge (shared email, bare first name → full
  name, company by name), identical names united first, exact de-duplication
  of deals/decisions/open threads (30 threads kept, round-robin) — because a
  model merge would have to emit the largest reply of the run (the haiku
  entities.json was 134 KB, ~35k tokens, minutes of decoding).
- Thoughts count against `maxOutputTokens`: a dynamic thinking budget spent
  15.7k of a 16k cap thinking and the JSON was cut, so the per-stage budget
  (sessions 0, roll-ups and entities 2048) is added on top of the answer cap.

Everything is resumable: a summary newer than its session's `extracted_at`,
a roll-up newer than its summaries and a current entities.json are skipped
unless `--force`; a session longer than `--max-chars` (3M) goes as ordered
parts (whole chunk files) with partials `<uuid>.<n>.json` and one merge
request, as the haiku prompt describes. 429/5xx/timeouts retry with
exponential backoff (5 attempts, Retry-After honoured); a 400, a blocked or a
truncated reply is counted and the run goes on (exit 1). `session`/`project`/
`cwd`/`sessions`/`generated_at` are filled from the index after the reply, not
trusted from the model. progress.py's recount runs every eight sessions and
after each stage, so status.json moves on its own. Output is counts, seconds
and token usage only.

SKILL.md: step 5 is the script, step 6 the haiku fallback, step 7 the count
check before staging; the acknowledgement, the completion message and the
"Never do" list say what is now true — "Your transcripts are processed by the
model providers your Sutando already uses; nothing is stored there and nothing
is uploaded to AG2 Space." — instead of "nothing leaves your Mac"; CLI rows
for the new flags. tests/import-claude-context-summarize.test.py (19 tests,
HTTP monkeypatched): no credential → exit 3; request body carries the schema,
the prompt fields and the transcript in order; 0600 summaries with the index
identity, skipped on re-run, redone with --force or after a re-extraction;
429 then success; five 429s give up; a 400 / a blocked / a truncated reply
counts and the run continues; parts + merge for long sessions and big
projects; entity groups merged in code; outputs land where finalize reads them
and `finalize --stage` renders the review; status recounted; the key never in
the output.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY

@yixuan-ag2 yixuan-ag2 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.

Re-reviewed at 54269fbb because my block has new work behind it. The blocker is still open, and the new summarize.py makes it worse rather than neutral. Everything else here is genuinely good, so I want to be precise about what is and is not owed.

The sk-ant- blocker is unaddressed at this head

Not inferred from commit messages — read at 54269fbb:

skills/import-claude-context/scripts/extract.py @ 54269fbb
  _EXTRA_SECRET_PATTERNS = { "Google API Key": re.compile(r"AIza[0-9A-Za-z_-]{35}") }   # one entry
  grep -c 'sk-ant'    -> 0
  grep -ci 'anthropic'-> 0
  control: grep -c 'redact' -> 15   (so the greps work; the zeros are absence)

secret_scanner.py is not in this PR's changed files, so the redaction path is byte-identical to the one I ran fixtures against. My earlier measurement carries forward unchanged: three controls (OpenAI / ghp_ / AIza) redact, sk-ant-api03-… survives in all three placements.

What changed is the blast radius, and it is the reason I am not just repeating myself

Before this push, a surviving sk-ant- key sat in a local 0600 dump. summarize.py now reads those same dumps and POSTs them off the machine:

ENDPOINT = "https://generativelanguage.googleapis.com/v1beta/models/{model}:generateContent"
  (dumps referenced 17× in summarize.py; transcript goes in the request body)

So the chain is now transcript → redact() → dumps/ → Google. A key that redact() misses no longer merely rests on disk; it is transmitted to a third party. And the SKILL.md description this PR rewrites now tells the owner "transcripts are processed by the model providers the Sutando already uses, nothing is stored there and nothing is uploaded to AG2 Space" — which is accurate about AG2 Space and about storage, and is exactly the sentence a reader would rely on when deciding this is safe.

Still one line:

_EXTRA_SECRET_PATTERNS = {
    "Google API Key": re.compile(r"AIza[0-9A-Za-z_-]{35}"),
    "Anthropic API Key": re.compile(r"sk-ant-[0-9A-Za-z_-]{20,}"),
}

and a fixture beside the three that already exist — the test file already asserts on secret material (assertNotIn(GHP)), so it is the same shape, not new machinery.

The rest of this push is good, and I do not want it read as blocked on quality

  • The counting fix is the honest kind. extracted counting sessions that wrote no dump (47 reported vs 43 on disk) is exactly the class of bug that shows up as a number nobody can reconcile later, and skipped_empty persisted in state.json so --new leaves them alone is the right fix rather than a filter at display time.
  • --counts-only now says what it cannot know — that sidechains and never-answered sessions are indistinguishable without a read, so empty/conversations are null there. A fast path that admits its own limits beats one that guesses.
  • summarize.py fails closed on no credential (exit 3, writes nothing, SKILL.md keeps the haiku path as fallback), and the key is header-only and kept out of error text.

Leaving CHANGES_REQUESTED in place — it is the same blocker, not a new one. Fix the pattern and I will clear it immediately; nothing else on my side is outstanding.

… pass the review gates; 100% diff coverage

Review round on #4127 (yixuan-ag2, marklysze) plus an owner scope change.

Scope change — the Gemini path is out. 54269fb added `scripts/summarize.py`
(direct, parallel Gemini Flash calls) and made the haiku subagents a fallback.
The owner dropped that from this PR: `summarize.py` and its test file are
deleted, SKILL.md's steps 5–7 are back to the haiku-subagent procedure as the
only procedure (one `model: haiku` worker per session, one per project, one
entities pass), the `summarize.py` CLI rows and the "Never do" mention are gone
("Summarise with anything but haiku" is back), and the CHANGELOG sub-bullet
that described the Gemini speed-up is removed. Two things from that commit are
kept on purpose: the owner-facing privacy sentence — "Your transcripts are
processed by the model providers your Sutando already uses; nothing is stored
there and nothing is uploaded to AG2 Space." — because it is the accurate
statement for the haiku subagents too (the old "nothing leaves your Mac" was
not), and the list-cap rules in prompts/project.md and prompts/entities.md,
which the haiku path benefits from as much as Gemini did. The same sentence
now replaces "nothing leaves the Mac" in the SKILL.md description, the
acknowledgement, the completion message and the "Never do" list, and in
docs/built-in-tools.md.

Blocker — Anthropic keys survived redaction. `_EXTRA_SECRET_PATTERNS` in
extract.py only knew `AIza…`; detect-secrets has no Anthropic plugin and
secret_scanner's whole-secret table is keyed by detect-secrets plugin types
(its own comment says a new type means enabling a plugin AND adding a row), so
the shape belongs with the other plugin-less shapes here rather than in
secret_scanner. `sk-ant-[0-9A-Za-z_-]{20,}` covers the current
`sk-ant-api03-…` form (the `api03-` segment is inside the character class)
without matching prose. The extract test's fixture now carries the key in a
USER turn as `export ANTHROPIC_API_KEY="…"` and asserts the key material and
the `sk-ant-` prefix are gone and the placeholder is in place, beside the
existing GHP/AIZA checks.

CI gates (scripts/review-checks.sh, REVIEW.md `checks:` block):
- hardcoded-paths: the index.py and session-recap extract.py docstrings spelled
  the stock projects dir literally; they now name it as
  `claude_home_path("projects", vanilla=True)`.
- prose-cap (comment runs ≤ 2 physical lines; docstrings out of scope): the
  four/three-line `# noqa: E402` import runs in extract.py and index.py are
  removed outright — ruff.toml ignores E402 globally, so the markers were
  decorative; the narrative comments in extract.py, finalize.py, index.py and
  src/context_resume.py are cut to the constraint (the incident history moves
  here); the test fixtures in the extract and stage tests lose one trailing
  comment each. index.py also loses a `t not in MESSAGE_TYPES` guard that was
  unreachable: `_TYPE_RE` only ever yields a META or MESSAGE type.

Diff coverage (scripts/coverage-gate.sh, 95% bar on changed Python lines): the
PR sat at 93% (88% in _common.py, 95% in finalize.py, 96% in extract/index).
Measured locally the same way CI does — every test file under `coverage run
--rcfile=.coveragerc` with detect-secrets installed, `coverage combine`, then
`diff-cover coverage.xml --compare-branch=origin/main --fail-under=95` — the
changed lines are now at 100% (1,479 lines, 0 missing). New
tests/import-claude-context-common.test.py covers the workspace resolution
through the config helper (subprocess patched; empty stdout refused), the
cross-drive `refuse_inside` branch, private JSON writes, the counts-only
status.json guard, `--since` parsing and `absorb_dash_values` from sys.argv.
The extract, index, finalize and stage suites gain the edge cases that were
uncovered: a `--projects` CSV string, a transcript deleted after the index, a
dump failure counted as an error; the stock default root, malformed meta
lines, a timestamp only on the last message, non-.jsonl files in a slug dir,
a `--since` that skips every project; the loaders on an empty data dir, an
unreadable fingerprint input, display-name fallbacks, memory-row append
without a trailing newline, the note/review renderer branches, junk entries in
the merge helpers and in `--forget`; the string selectors for stage/commit/
discard, the human (non-JSON) CLI lines, `--purge-dumps` alone, a roll-up that
vanished after staging, and a `summarized_at` older than its extraction.

Also run green: tests/session-recap-extract.test.py, tests/context-resume.test.py,
tests/credential-resolver.test.py, scripts/lint-claude-home-path.sh,
scripts/lint-skill.py --all --strict, ruff on the changed files, the python39
compatibility checks.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Addressed at 76ed4624:

  • sk-ant blocker: _EXTRA_SECRET_PATTERNS gains Anthropic API Key (sk-ant-[0-9A-Za-z_-]{20,}, covers the current sk-ant-api03-… form); the extract test carries the key in a USER turn as an env export and asserts the material and the prefix are gone (assertNotIn, beside GHP/AIZA).
  • Scope change: the Gemini path (54269fb) is dropped from this PR at the owner's request — summarize.py and its test are deleted, haiku subagents are the only procedure again. The corrected privacy sentence stays (it is the accurate statement for haiku too) and now replaces every "nothing leaves the Mac" in SKILL.md and docs/built-in-tools.md; cinny#958 carries the identical sentence.
  • review-checks: hardcoded-paths (both docstrings now name claude_home_path("projects", vanilla=True)) and prose-cap (all flagged blocks trimmed; narrative moved to the commit message) pass locally with scripts/review-checks.sh.
  • Diff coverage: measured with scripts/coverage-gate.sh's own invocation against origin/main: 100% of changed lines (new tests/import-claude-context-common.test.py + edge cases in the extract/index/finalize/stage suites).
    Merge order: this PR → ag2space-cinny-desktop#545 → cinny#958.

@yixuan-ag2
yixuan-ag2 dismissed their stale review September 10, 2026 18:47

Dismissing my own stale CHANGES_REQUESTED: the blocker is fixed at 76ed462. _EXTRA_SECRET_PATTERNS now carries sk-ant-[0-9A-Za-z_-]{20,} alongside the Google pattern, with a realistic fixture (sk-ant-api03- + 20+ chars) placed inside an export ANTHROPIC_API_KEY= line and assertNotIn("sk-ant-", text) asserting the material is gone rather than counting redactions. Verified at head, with a control so the greps are known to work. Dismissing rather than approving because 2 checks are still running and 2 required contexts are absent from the rollup — my objection is resolved, which is a separate thing from vouching for this head.

@marklysze marklysze 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.

Re-verified at the current head (76ed4624), independently of the room's back-and-forth on whether this is still blocked.

Both things yixuan-ag2's 17:31Z review flagged (at 54269fbb) are resolved at this head — checked directly, not from commit messages:

  • _EXTRA_SECRET_PATTERNS in extract.py now includes "Anthropic API Key": re.compile(r"sk-ant-[0-9A-Za-z_-]{20,}"). Ran redact() myself against a real-shaped key (sk-ant-api03-...) — it's stripped to [STORED-IN-KEYCHAIN-Anthropic API Key].
  • summarize.py (the new Gemini path yixuan flagged as the reason a surviving key would now leave the machine, not just sit on disk) is gone from the tree at this head — confirmed via a 404 on the file.
  • tests/import-claude-context-extract.test.py has the matching fixture (ANT = "sk-ant-api03-" + ...) and assertion (assertNotIn("sk-ant-", text)) yixuan's review asked for. Ran the suite: 17/17 pass.

One state-tracking note so nobody's waiting on the wrong thing: GitHub's reviewDecision currently reads REVIEW_REQUIRED, not CHANGES_REQUESTED — yixuan's 54269fb review is recorded as COMMENTED (their prose said "leaving CHANGES_REQUESTED in place," but the submitted review type is comment, and dismiss-on-push cleared their earlier formal block on this repo). So there's no live formal block on this PR right now, but yixuan was explicit ("fix the pattern and I will clear it immediately") about wanting to confirm and close it themselves — @yixuan-ag2, the fix is in and I've verified it holds; over to you to confirm and clear.

…personal sessions back

Two product fixes on #4127, both from a live run of the import.

1. Existing people were overwritten. `people__upsert_person` de-duplicates
on slug > email > name and REPLACES `doc` when one is given; the finalize
payloads always carried a fresh `doc`, so a person the owner's store already
held lost their dossier to a page of Claude Code citations. The scripts cannot
reach the station tools, so the store listing now travels as a file:

- `finalize.py --stage --known-people FILE` takes the coordinator's copy of
  `people__list_people` (a JSON array of id / slug / name / email /
  identifiers.emails — never dossiers) and matches every staged person by any
  email (case-insensitive) first, then by normalised name (`_norm_name`, NFKD
  with the combining marks dropped, casefold, whitespace collapsed — so an
  accented spelling and a plain one are the same person). A match becomes an
  UPDATE payload: the store's `slug`/`id`, `identifiers.emails` merged
  (store spelling first), `last_interaction_at`, `source: claude-import`,
  `doc_append` — one `## Imported from Claude Code (<date>)` section — and NO
  `doc`. A new person keeps the full dossier as before, `existing: null`.
  A normalised name two store entries share is ambiguous: it goes to the
  manifest's `people_ambiguous` and the review's "Needs your call", never into
  people.json, so it cannot be upserted by accident.
- The 25 cap now applies to new people only; existing people ride on top.
- `--people-doc-merge --existing-doc F --append F` prints the dossier with the
  section merged in: a section under the same heading is replaced in place,
  otherwise the section is appended — idempotent, so a re-import of the same
  day never stacks sections.
- The listing used at staging is copied to `staged/known-people.json` (0600)
  and reused by every re-stage (`--commit --projects`, `--discard`,
  `--forget`, the session commands) and by the commit, which writes the
  approved payloads to `<data-dir>/people.json`; `--people-json` prints the
  staged copy while a review is pending, else that approved copy, and
  recomputes only with `--projects` (against `--known-people`, else the staged
  listing). Before this, a `--people-json` after the final commit recomputed
  from the entities with no store check at all — the exact path that
  overwrote a dossier.
- Without `--known-people` the manifest records `known_people: "not checked"`,
  every payload is `existing: null`, and review.md says "Store not checked
  (People tool unavailable)"; SKILL.md tells the coordinator to upsert nobody
  in that case.

2. Personal sessions reached the digest. The session-summary schema gains
`personal` (true when the session is mainly about the owner's private life —
the prompt lists the categories and says to mark personal when unsure) and
`personal_reason` (a generic category, at most 6 words, never a quote).
finalize.py HOLDS such a session: its summary is dropped from the inputs, its
citations are stripped from the entities before the merge, it is subtracted
from every session count, left out of the notes' session lists, never given a
`summarized_at`, and appears only as `date · reason` in a "Held back as
personal (N)" section at the bottom of review.md (with the include/hold
instructions) and as `{project, session, date, reason, held: personal}` in
the manifest — never a title, never a summary line. The stage test plants a
canary string in every field of a held session and asserts it surfaces in no
staged file, note, memory file, payload or manifest.

Owner overrides live in state.json as `personal_override: include | hold`
per session, so they survive re-stages and a later re-summarisation:
- `--include <date|uuid>` / `--include-personal` lift a hold;
- `--hold <date|uuid>` holds a summarised session the classifier missed
  (refused once it has landed — the note is on disk; `--forget-session` /
  `--forget <slug>` are the undo there);
- `--forget-session <date|uuid>` deletes the summary (and partials), dumps
  and state entry; a second call on the same uuid is a no-op, not an error.
A date shared by two sessions is refused with both uuids; a uuid or its first
8+ characters names one exactly. Each command re-renders a pending review.

Stale roll-ups. The project roll-up is model-written from the session JSONs
the coordinator lists, so its text may carry a session that is now held, or
lack one the owner just included. `stale_rollups()` judges each roll-up on
the `sessions` list it wrote: a held uuid in it, an owner-included uuid
missing from it, or a listed uuid whose summary was forgotten marks the
project stale. A stale roll-up is staged REDACTED (name, dir, status and the
session list survive; every sentence the model wrote is replaced by a
placeholder in the note, the review paragraph and the memory preview), the
review's meta line says why, the manifest carries `stale_rollups`, and
`--commit` refuses those projects until the coordinator re-runs the roll-up
and stages again. A new session the roll-up never saw is deliberately NOT
stale — a model that omits a uuid from its list would otherwise block the
project forever.

SKILL.md: step 5 explains the flag, step 6/7 pass only non-personal
summaries to the roll-up and entities prompts (prompts/project.md and
prompts/entities.md say so too), step 8 writes `$DATA/known-people.json` from
`people__list_people` and passes `--known-people`, step 10 handles `existing`
payloads (get_person → --people-doc-merge → upsert with the same slug/id) and
gains the include / include personal / hold / forget <date> replies, step 11
reports held and updated counts and asks about ambiguous names; two new
"Never do" bullets; CLI and file tables. `--forget-session` is documented as
transient: the index row stays and a `--new` run re-summarises the session
unless the transcript is gone or the owner holds it.

Gates: scripts/review-checks.sh PASS on the origin/main diff; diff coverage
(coverage 7.16 + diff-cover, measured as scripts/coverage-gate.sh does) 100%
over 1,627 changed lines; ruff clean; lint-skill --all --strict clean;
check-python39-compat clean and both suites green on Python 3.9.6 as well.
tests: import-claude-context-{common 9, extract 17, finalize 28, index 19,
progress 3, prompts 3, stage 39}, session-recap-extract 13, context-resume
17, credential-resolver — all green.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HRt4UBib3zRnBt4TSGm4zY
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Two more product fixes at c33ecbac, both found on the live run today:

  • People store dedupe: --stage --known-people matches staged people against the owner's store (email, then normalised name); existing people become update payloads with an appended ## Imported from Claude Code section and merged identifiers, never a replacement doc; ambiguous names go to "Needs your call" and are never upserted automatically; the digest splits "already in your store" from "new".
  • Personal-session hold: the session schema gains personal/personal_reason; held sessions contribute nothing to memory, notes, entities or people and appear in the digest by date + generic reason only; owner replies include <date>, include personal, hold <date>, forget <date>; --commit never lands a held session and refuses a stale roll-up.
    Gates pass; diff coverage 100% (1,627 changed lines). All fixtures are invented.

@marklysze marklysze 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.

Verified at head e0f79d9e (tip is a merge from main pulling in unrelated proactive-loop skill files — confirmed via git compare c33ecbac...e0f79d9e that nothing in this PR's own content changed, so this reviews the actual content commit c33ecbac).

New commit on top of the already-verified redaction fix, addressing two real bugs found from a live run:

  • Existing people no longer get overwritten. people_payloads now omits doc entirely for a matched existing person, replacing it with doc_append (a merge-safe dated section) — verified by reading the code and mutation-testing: forcing doc to also be set on the existing-person branch breaks exactly 3 targeted tests (test_email_match_gives_an_update_payload_without_a_doc, the diacritics-match test, and the restage/commit persistence test), nothing else.
  • Personal sessions are held back. held_sessions() correctly drops any summary flagged personal: true (absent an owner --include override) from every downstream artifact. Mutation-tested by disabling the personal: true check: breaks 2 tests in stage and 1 in finalize, each asserting the held session's canary strings don't leak into staged output.

Ran all 10 named test suites directly — exact counts match the commit message: common 9, extract 17, finalize 28, index 19, progress 3, prompts 3, stage 39, session-recap-extract 13, context-resume 17, credential-resolver pass. ruff clean. review-checks.sh (prose-cap + hardcoded-paths) PASS on the full diff.

Could not reproduce the 100% diff-coverage figure locally for the same reason noted on #4126 — this repo's coverage-gate.sh requires the whole tree's suite green first, and this sandboxed host is missing tmux and full screen-capture access for unrelated suites. CI's own gate passed (10m27s), which is the authoritative number.

19/19 CI checks green, mergeable, no conflicts. Approving.

@yixuan-ag2 yixuan-ag2 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.

My blocker is cleared — verified at head e0f79d9e, by executing the pattern rather than reading the commit message. Approving.

The sk-ant- blocker: fixed

76ed4624 added the pattern; I ran it against the exact three placements that defeated the old one, plus controls:

"Anthropic API Key": re.compile(r"sk-ant-[0-9A-Za-z_-]{20,}")

bare, own line          -> Anthropic API Key      OK
inline in prose         -> Anthropic API Key      OK
quoted inside json      -> Anthropic API Key      OK
short sk-ant (<20 tail) -> NO MATCH               OK   (not over-broad)
AIza… (google control)  -> Google API Key         OK   (pre-existing rule intact)
plain prose             -> NO MATCH               OK
substitution: 1 replacement -> "my key is [STORED-IN-KEYCHAIN-Anthropic API Key] ok"

6/6, including the negative controls — so it redacts without being greedy, and it didn't break the Google rule it sits beside.

And the blast radius I raised is gone entirely

My previous comment's real objection was not the pattern alone — it was that summarize.py had begun POSTing those same dumps to generativelanguage.googleapis.com, which turned a local 0600 leak into an exfiltration. At this head, across all 20 changed files: zero occurrences of generativelanguage, urlopen, or requests.post. The Gemini path is not narrowed, it is removed. That is the stronger fix and I'd rather say so than treat it as merely adequate.

On c33ecbac, which I had not reviewed

It is the largest thing here (+680/-127 in finalize.py), so I looked at the property most likely to bite: name-based matching. Matching people by normalised name is the kind of fix whose failure mode is worse than the bug it fixes — overwriting one dossier is recoverable, silently merging two different people is not.

It's handled, and handled the right way round. match_known() returns (existing, ambiguous); a name shared by two store entries goes to ambiguous, select_people() returns those separately and never as a candidate, and they surface under "Needs your call" in review.md rather than landing in people.json. 20 references through finalize.py, and test_name_match_folds_diacritics_and_two_entries_are_ambiguous covers the collision together with diacritic folding. Email is tried before name, and _emails_of unions email / emails / identifiers.emails case-insensitively, so the cheap unambiguous key gets first refusal. Escalating to a human on ambiguity instead of guessing is the correct default for this.

Scope of this approval, stated so nobody over-reads it

I verified: the redaction pattern (executed), the removal of the network path (all 20 files), and the ambiguity semantics of the new known-people matching (read + tests named). I did not line-by-line review all 680 new lines of finalize.py; @marklysze did, including mutation-testing the existing-person branch, and I am relying on that for the remainder rather than re-deriving it.

One note for whoever counts the gate: @marklysze's approval doesn't move reviewDecision here — it needs two approvals from write-access logins, and this now has one (mine). One more from any of john-the-dev, jsun-m, qingyun-wu, liususan091219 or sonichi lands it.

@qingyun-wu qingyun-wu 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.

Changes requested at e0f79d9e. I reproduced three blocking failures through the production importer using only synthetic transcripts and temporary state. All 118 importer tests and 17 context-resume tests pass, so these need additional boundary coverage.

commit A; change A rollup; stage/commit only B:
  B review contains changed A text=False; resulting core memory contains it=True
forget ../outside with an unrelated notes/outside.md:
  unrelated note removed=True
synthetic Anthropic-shaped token through scan_session + session_record:
  survives title=True, first_prompt=True; extractor redacts the same token

The inline comments identify the fixes. Worst case is importing unapproved personal context, deleting unrelated workspace notes/directories, or handing a credential to the summarizer despite the redaction promise. This is an optional skill, but consent to import selected history does not authorize those additional effects. The approved people.json export also stays byte-identical after forgetting A and still cites A; regenerate/revoke that payload as part of forgetting so a later people export cannot reintroduce it. No real user transcript, credential, memory or note was modified.

Full current diff and activated surrounding paths inspected; focused tests and added-path scan run on this head. The forbidden-path positive control is rejected. Current hosted check rollup and CLA are green. This review does not waive required CI, CLA, parent order, or the two-maintainer approval gate. No merge, contributor-branch edit, or live owner-runtime change.

Reviewed by Qingyun’s Codex automation.

…etention

The consent/acknowledgement sentence said the model providers store nothing.
That is a claim about their retention terms, which the skill cannot
guarantee; what the API terms do give is that the data is not used to train
their models. Every occurrence in SKILL.md (description, acknowledgement,
completion message, never-do) and docs/built-in-tools.md now reads
"…processed by the model providers your Sutando already uses and are not used
to train their models; nothing is uploaded to AG2 Space." cinny#958 carries
the identical sentence at 82a9ddc0. Raised as non-blocking by yixuan-ag2.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

@yixuan-ag2 @marklysze @vidhuUC re-review requested at e212391b. Since your last looks: c33ecbac (People-store dedupe: --known-people matching by email then normalised name, existing people get an appended section + merged identifiers and never a replacement doc, ambiguous names go to "Needs your call"; personal-session hold: personal/personal_reason in the session schema, held sessions never reach memory/notes/entities/people or the digest body, owner replies include <date> / include personal / hold <date> / forget <date>), e0f79d9e (main merged), e212391b (the disclosure sentence now claims the training terms, not provider retention — yixuan's point on #958; cinny#958 has the identical sentence at 82a9ddc0). Re-verified by execution at head: redact() strips sk-ant-api03-… in an assistant turn, a user turn and an ANTHROPIC_API_KEY= export (GitHub/Google/OpenAI controls still redact); all seven tests/import-claude-context-*.test.py suites green; review-checks.sh PASS; lint-skill strict 0 errors; CI green incl. the coverage gate. One counting approval from a repo writer is still needed to merge.

@qingyun-wu qingyun-wu 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.

Changes requested at e212391b. The late change is documentation-only; the importer scripts are byte-identical to the head I reviewed. I read the entire current diff and replayed the production boundary probes on this checkout:

stage/commit A; change A; stage/commit only B:
  B review includes changed A=False; core memory acquires changed A=True
forget ../outside removes unrelated notes/outside.md=True
extract redacts synthetic Anthropic token=True; index metadata redacts it=False
forget A revokes its approved people export=False

The three inline blockers in my preceding review therefore remain: use approved snapshots for unselected projects (finalize.py:1467), validate/confine forget targets before deletion (:1523), and apply the same secret-redaction policy to index metadata (index.py:210). The stale approved people payload must also be invalidated or regenerated when forgetting. The new privacy wording does not repair these code paths. No duplicate inline threads added.

The unchanged code’s 118 importer tests and 17 context-resume tests passed earlier in this pass; the fresh production probes above still expose the missing boundary cases. Added-path scan passes with the previously rejected forbidden-path control. Current hosted CI is still running and this SHA has no license/cla status in the inspected rollup. Worst case remains importing unapproved personal context, deleting unrelated owner data, or exposing a credential to summarization. No real user data, live runtime or contributor branch was modified. Approval requires those behavioral fixes and their boundary tests; passing CI alone is insufficient.

Reviewed by Qingyun’s Codex automation.

yixuan-ag2
yixuan-ag2 previously approved these changes Sep 10, 2026

@yixuan-ag2 yixuan-ag2 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.

Re-anchoring my approval at e212391b — given at the previous head, with a content commit after it, so it was counting on text I had not read. Same reason as #4126: stale approvals are not dismissed on push here.

Reviewed the delta only. e212391b is documentation: the SKILL.md description and docs/built-in-tools.md now claim the providers' training terms rather than provider retention. That is the narrower and more defensible claim — retention is a promise about what a third party stores, which this project cannot keep on their behalf, while "not used to train" is the term the API actually offers. It moves in the same direction as the consent-copy finding I raised on cinny-webclient#958, and it does not re-introduce the unqualified "nothing is uploaded" that I blocked on earlier here.

No production code in this commit; the redaction and known-people paths I verified at e0f79d9e are untouched. Approval re-stamped at this head. @qingyun-wu's block remains the gate.

@qingyun-wu qingyun-wu 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.

Changes requested at 72c215945. I caught this new head before posting the earlier draft; that draft was not posted. The original rollup, traversal and redaction failures are fixed, and the new approved entity inputs now also prevent an A-only person changing during a B-only commit. Forget removes A’s approved People citations. Seven importer suites pass (10/17/34/21/3/3/45), plus the earlier 17 context-resume checks; added-path scan passes with a rejected forbidden-path control.

One approval-boundary case remains in finalize.py:1754-1756: a person shared across selected and unselected projects can acquire fields the digest never showed. Production stage/commit replay with Ada cited twice by A and once by B:

commit A: approved Ada role = CTO
change entities.json role to UNREVIEWED_ROLE; stage only B
B review.md shows new role: false; staged people.json: []
commit only B: approved people.json contains UNREVIEWED_ROLE: true
explicitly stage A instead: review.md shows UNREVIEWED_ROLE: true

B’s one citation fails the two-citation floor, so the review shows no proposed update for Ada. approved_entities_after nevertheless takes Ada’s current fields from B, _absorb keeps those fields, and adding A’s approved citations makes her eligible for export. The code comment calling these fields “the entities the owner just reviewed” is not true for that path.

Stage the actual post-union People changes that commit will publish, or keep approved fields until the corresponding update has appeared in the digest. Add this shared-person / below-floor-selected-project case. Worst case is publishing unreviewed personal data despite a B-only approval. The three original threads can be resolved; this distinct remaining approval gap blocks approval. The late update also includes a main-base refresh; no contributor branch or live owner data was changed. Required CI, CLA and two maintainer approvals remain separate gates.

— Qingyun’s Codex review automation.

…t the commit publishes

qingyun-wu's PR #4127 review found the last P1: with a person cited twice by an
approved project and once by the selected one, `--stage` computed a *selected-only*
People set — below the two-citation floor, so the person was absent from
`staged/people.json` and from `review.md` — while `--commit` unioned in the approved
project's citations (`approved_entities_after` / `_absorb`) and published a role change
nobody had reviewed. Changing that person's role in `entities.json` between two commits
therefore landed in `people.json` without the digest ever showing it.

Invariant now enforced: what `--commit` publishes to `people.json` is exactly what the
digest showed.

- `--stage` computes the People set the way `--commit` will publish it — the selected
  projects' live citations and fields unioned with every other landed project's approved
  citations and fields (`approved_entities_after`, the same call the commit makes). So a
  person who clears the floor only through the union, or whose fields change, is now in
  `staged/people.json`, and the digest's new "Updates to people already in your export"
  section (`published_field_diffs`) lists each shared person with the concrete field diff
  (`role: CTO → …`, name change, gained emails) and citation count. A person whose
  published fields equal their approved entry has nothing to review and is not listed.
- A full `--commit` publishes the staged People set verbatim (fingerprint-guarded like the
  rest of the staged set; a `--projects` subset re-derives its slice the same way `--stage`
  did). No recomputation from live `entities.json` at commit time — a stale staged set is
  refused as it already is for stale summaries.
- A person in the approved inputs but not listed in the digest keeps the approved fields
  byte-for-byte after commit (the reviewer's case: if the diff is not shown, approved wins).
  Fixed the misleading commit comment ("the entities the owner just reviewed").
- Tests (stage + finalize suites), each failing on 72c2159: the exact replay — commit A
  with Ada role CTO (2 cites in A, 1 in B); change Ada's role in `entities.json`; stage only
  B → `review.md` previews the role change and `staged/people.json` carries it; commit B →
  `people.json` publishes exactly the staged role. A second variant asserts that a change the
  digest does not render (the person cited only by the unselected landed project) keeps the
  approved role after commit. `published_field_diffs` gets a direct unit test. Every existing
  snapshot / revocation / known-people test stays green.
- SKILL.md steps 8 and 10, a Never-do bullet, and a CHANGELOG-PENDING sub-bullet.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu the remaining P1 (shared person across an approved and a selected project) is fixed at ac96a5c6: staging previews the exact post-union People set, the digest shows each shared person's field diff, and commit publishes the staged set verbatim — what lands is what was shown, and anything not shown keeps its approved fields. Gates PASS, diff coverage 100%, suites stage 47 / finalize 36 (167 tests across the import battery). Head for re-review: ac96a5c6. @yixuan-ag2 @marklysze FYI.

@qingyun-wu qingyun-wu 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.

Changes requested at ac96a5c65846f37875763a5bd2fd2ad2a01f38bd. The previously reported short-role/shared-person case is fixed: staging B now shows the post-union Ada change and a full commit publishes it. Two adjacent consent cases remain; the inline comments give their locations and remedies.

Independent production-CLI replays used synthetic histories and temporary workspace/memory directories only:

case                         digest shows change/person   published
short shared-role change     true                         true
same role prefix (61 chars)  false; no field-update row   changed suffix
stage A+B; commit only B     false; absent staged people  Zed Hidden
control: stage B; commit B   true; present staged people  Zed Hidden

The cap replay uses 25 A-only people and one B-only person, each with two distinct session citations. The A names sort ahead of B's. Full staging excludes B's person, but the subset branch recomputes its top 25 and imports that unseen person. Restrict the approved subset to what was actually staged/shown, or require a new subset digest and approval before any sink mutation.

For the field replay, the approved/new role strings are Senior director of international engineering and operations, APPROVED and the same prefix plus UNREVIEWED. Comparing 60-character display snippets suppresses the update entirely; the 80-character published role still contains UNREVIEWED. Compare the values that will be published, then render an observable delta or preserve the approved value.

Validation on HEAD: 137 importer tests plus 17 context-resume and 13 session-recap tests pass (167 total). With the previous reviewed 72c215945 finalize.py and these same tests, both stage and finalize suites fail once and error once (the new diff helper is absent); the independent short-role replay also changes from hidden at the previous head to shown at HEAD. The two remaining failures above reproduce at both versions. Importer scripts are new relative to current main f71f1aae; the shared parser changes retain the passing adjacent suites. Added-path/root/prose checks pass, and a synthetic forbidden-path control is rejected.

Worst-case disruption is importing personal data beyond the concrete digest the owner approved. This optional skill needs the two approval-boundary fixes before approval. Hosted checks are green; this head currently has no license/cla status. No real transcript, owner memory, live runtime or contributor branch was changed, and no merge was performed.

Reviewed by Qingyun’s Codex automation.

… digest listed; field diffs compare the published values

Two consent-boundary cases from the PR #4127 review (qingyun-wu, at ac96a5c),
both of the same class as the earlier fixes: what a commit publishes to the
People export must be exactly what a digest showed and the owner approved.

1. `--commit --projects <slug>` (finalize.py, the subset branch of commit())
   recomputed the capped top-25 candidate set over the narrowed projects. With
   25 A-only people and one B-only person (two citations each; A's names sort
   first), a full stage showed only the 25 A names — the cap hid the B-only
   person — yet "bring in beta" ranked the B-only person first inside beta and
   published them: a person absent from both staged/people.json and review.md
   landed in people.json. Now a subset publishes only the people
   staged/people.json lists (the reviewed set), each re-rendered from the
   landed projects' citations — the chosen projects' live ones plus the
   approved citations of projects landed earlier, as before — and kept only
   while those still clear the two-citation floor; nobody is added, nobody is
   re-ranked (`reviewed_subset_payloads` / `within_reviewed`). The same guard
   closes the second door: `--people-json --projects <landed slug>` used to
   recompute from the approved inputs, which cite everyone in those projects,
   so it would have printed the unpublished person for the coordinator to
   upsert; it now prints only people the export published. A project-only
   view (the people another project's names pushed past the cap) needs
   `--stage --projects <slug>` and that digest's yes — SKILL.md step 10 says
   so under "bring in <slug>".

2. `published_field_diffs` compared 60-character display snippets, so an
   approved role "Senior director of international engineering and
   operations, APPROVED" and the same prefix + "UNREVIEWED" produced no
   update row while the 80-character published role carried the new suffix.
   The comparison is now on the values the dossier publishes (one
   `published_field`, PUBLISHED_FIELD_LIMITS: role/company 80, relationship
   200 — the renderers use it too, so the two cannot drift apart again), and
   `field_delta` renders an observable change: both values whole up to 120
   chars, otherwise the shared prefix elided to `…` (backed up to a word
   start, so a tail is never empty when one value merely extends the other)
   with both tails. A row is never suppressed because a preview would look
   alike.

Tests (each fails on ac96a5c, passes now):
- stage suite `TestSubsetCommitStaysInsideTheDigest`: the exact 25+1 replay —
  after a full stage, `--commit --projects beta` publishes nobody the digest
  did not list (export empty, `--people-json --projects beta` empty, the
  re-staged Alpha digest still hides the person, the final commit never lands
  them); the control `--stage --projects beta` shows and then publishes them;
  a subset cut of a shown person to the landed citations (Ada keeps her
  UPDATE payload, loses Beta's citation until Beta lands); `within_reviewed`
  matching rules.
- finalize suite: the same-prefix replay (review.md carries the role row with
  the differing tail; the commit publishes it) and `field_delta` /
  `published_field_diffs` rendering: long shared prefix elided, one value
  extending the other, no shared word, a change past the dossier's own cut
  (no published change, no row), relationship compared at 200.

Gates: review-checks PASS (prose-cap 2), ruff clean, lint-skill --all --strict
0/0, diff coverage 100% (2,027 lines), suites common 10 / extract 17 /
finalize 38 / index 21 / progress 3 / prompts 3 / stage 51.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu both remaining cases fixed at 9c437622: (1) a subset commit publishes only people the current digest listed, cut to the landed projects' citations — never a recompute over the narrowed projects; (2) field diffs compare the full published values and show an elided-prefix delta, so a change beyond the preview width can't be suppressed. Both replays (25 A-only + 1 B-only under the cap; same-prefix role suffix) are failing-then-passing tests. Closing/reopening once more for the missing license/cla status.

@qingyun-wu qingyun-wu 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.

Changes requested at 9c437622668b18d525cd573bd9764ef5d6077e9c.

The two previous findings are fixed: the capped partial commit stays inside the staged names, and same-prefix role changes appear in the digest. Current stage/finalize suites pass 51/38; running those current suites with the previous ac96a5c6 finalizer gives one behavioral failure in each, plus one missing-new-helper error in each (the missing helpers are not behavioral proof). I am resolving those two old threads.

One approval boundary remains in the revocation path (finalize.py:1411-1414). Using the new 25 Alpha-only + one Beta-only fixture through the production CLI:

--stage; --commit:
  digest/export = 25 Alpha names; Zed Hidden absent
--forget-session <first Alpha session>:
  rc=0; export = [Zed Hidden]; people_revoked=24; staged_remaining=0
fresh fixture, --stage; --commit; --forget alpha:
  rc=0; export = [Zed Hidden]; people_revoked=24; staged_remaining=0
CONTROL --stage --projects beta; --commit:
  digest/export = [Zed Hidden]; no unseen name added

Both revocation cases also reproduce at ac96a5c6; they are a remaining path in this feature, not a regression caused by this last patch. refresh_people_export re-ranks all stored entities instead of restricting itself to the previously published people, bypassing the newly shared reviewed-set guard. A request to remove information must not authorize a new People payload. Apply the same reviewed-set boundary to revocation and test membership, not only the count reduction.

All seven importer suites plus session-recap/context-resume pass (173 tests total). Added-path/root/prose scans pass with a rejecting forbidden-path control. Current hosted checks are not all green: the clean-install job failed and another job is still running; I did not diagnose that failure here. Worst case is unseen contact information entering the approved export during deletion; retaining only previously published identities until another reviewed commit mitigates it. No real history, People store, contributor branch or runtime was changed.

Reviewed by Qingyun’s Codex automation.

@john-the-dev john-the-dev 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.

APPROVE at 9c4376226. Both of @qingyun-wu's approval-boundary issues are fixed, and I reproduced each myself rather than reading the commit.

I said earlier this was worth reviewing "once qingyun-wu or someone confirms the new commit closes both gaps." @bassilkhilo-ag2 pointed out I am someone. That was right — declining to check a commit because nobody else had yet is deferral, not caution, so this is my own verification and not a countersign of anyone else's.

Issue 1 — the subset commit no longer publishes an unstaged person

Rebuilt the shape from the review: 25 A-only people sorting ahead of one B-only person, stage A+B, commit only B.

within_reviewed(reviewed=[Ada 00..24], payloads=[Ada 03, Zed Hidden, Ada 11])
  -> ['Ada 03', 'Ada 11']

  'Zed Hidden' published?                      False   <- the digest never showed them
  CONTROL reviewed people still come through?  True
  CONTROL order follows `reviewed`, not payload order?  True
  name-only match (email differs)              ['Ada 03']
  VACUITY reviewed=[] -> []                    publishes nothing

The vacuity row is the one I most wanted: if the approved file is missing or empty, load_json(approved, []) yields [] and the commit publishes nobody. It fails closed, which is the only acceptable direction for a consent gate.

Issue 2 — the field diff compares published values

prefix 61 chars; old = prefix+"APPROVED" (69), new = prefix+"UNREVIEWED" (71)

published_field(old) == published_field(new)   False    <- was equal at 60 chars, hiding the change
field_delta -> 'Senior director of … operations, APPROVED → Senior director of … operations, UNREVIEWED'
changed suffix visible?  APPROVED True   UNREVIEWED True

I also checked the layer above before calling anything a defect: field_delta formats unconditionally, but published_field_diffs guards with if old != new and if diffs, so an unchanged field emits no row. My first "control" ran against the formatter and looked like noise — it was me testing the wrong layer.

It is structural, not a guard

I went looking for the way round it, because a filter at one call site is a patch:

within_reviewed     1 definition; call sites 1245 (helper) and 2256 (commit) — both intersect
cap=None            reachable from exactly one line, 1244, whose result is handed straight
                    to within_reviewed on the next line
existing + new[:cap]  1089 — the capped selection that builds the digest, unchanged

The uncapped pool exists only to be intersected with the reviewed set. There is no path where a subset commit selects people without passing through within_reviewed, which is what makes "a re-cut never adds a person" a property rather than a promise.

Both suites green at head: import-claude-context-finalize and import-claude-context-stage.

Scope — what this approval does and does not cover

+7128/-2 across 20 files is a lot, and I did not read all of it. I reviewed the consent boundary: finalize.py's selection and diff paths, their call graph, and the two replays above. I have not reviewed the transcript parsing, the session-recap and context-resume integration, or the SKILL.md workflow beyond the paragraphs describing these two behaviours.

That is a deliberate scope, not an oversight, and I would rather name it than let an approval read as broader than the work behind it. For a skill whose whole function is importing a person's history, the approval boundary is the part that most needed an independent pair of eyes, and it is the part I checked.

@qingyun-wu qingyun-wu 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.

Re-reviewed at 9c437622668b18d525cd573bd9764ef5d6077e9c. Both blockers of my standing review are verified fixed, each with a positive control that fails at the previously reviewed head and passes here. Approving; this APPROVE replaces my CHANGES_REQUESTED latch — a comment would have left it blocking.

One correction on which head was blocked. My latest decisive review is at ac96a5c65846f37875763a5bd2fd2ad2a01f38bd (2026-09-11 10:42Z), not at 72c215945. ac96a5c6 (the --stage post-union preview) had already cleared the finalize.py:1754-1756 shared-person/below-floor finding — I verified and said so in that review. What that review left open was two different cases, and 9c437622 is the single commit answering them. Control runs below are therefore ac96a5c6 → 9c437622.

[P1] Keep a partial commit inside the reviewed People set — fixed

commit()'s --projects branch called people_payloads(..., projects=set(committed)), re-running the 25-person cap over the narrowed projects, so a subset commit could publish someone the combined digest's cap had hidden. At this head it calls reviewed_subset_payloads() → within_reviewed(), which keeps only people staged/people.json listed, cut to the landed citations and re-checked against the floor; --people-json --projects <landed> gets the same restriction (finalize.py main()).

Independent replay through the production CLI (finalize.py --stage / --commit --projects beta / --people-json), with my own shape rather than the fixture in the new test — 30 Alpha-only people at 2 citations each, one Beta-only person (Mmm Hidden Person, 2 citations) whose name sorts in the middle of them, cap 25:

                                ac96a5c6 (control)     9c437622 (head)
--stage           staged_n       25                     25
                  hidden shown   False                  False        <- the cap hides them in both
--commit --projects beta
                  published_n    1                      0
                  hidden published  True                False
--people-json --projects beta
                  n / hidden     1 / True               0 / False

So the leak reproduces at the reviewed head and is gone here, and --people-json — the payload list the coordinator actually upserts from — leaked the same unseen person and no longer does.

The author's own regression test discriminates too. Head's tests/import-claude-context-stage.test.py run against ac96a5c6's finalize.py:

FAIL: test_bring_in_beta_publishes_nobody_the_full_digest_did_not_list
AssertionError: Tuples differ: (0, 1, 1, 1) != (0, 1, 1, 0)   # people published: 1, expected 0
ERROR: test_within_reviewed_matches_by_email_then_name_in_the_reviewed_order
       AttributeError: module 'ici_finalize' has no attribute 'within_reviewed'

Collection moved by the number of tests added, so these ran rather than being skipped: stage Ran 47 → Ran 51, finalize Ran 36 → Ran 38.

[P2] Compare published field values before truncating the preview — fixed

published_field_diffs() compared _one_line(..., 60), so two roles sharing a 60-character prefix produced no update row while the dossier published the 80-character value with the changed suffix. At this head the comparison is published_field() at the dossier's own limits (PUBLISHED_FIELD_LIMITS role/company 80, relationship 200) and the row renders through field_delta(), which elides a shared prefix rather than a tail.

Independent probe with my own strings (67-character shared prefix, suffix OK → REVOKED, both values inside the published 80):

                                        ac96a5c6 (control)   9c437622 (head)
published_field_diffs rows              []                   [('Bea Zed', ['role: …OK → …REVOKED'], 3)]
"REVOKED" present in the digest section False                True

The remedy asked for was "compare the complete publishable values and display the changed portion" — that is what published_field + field_delta do, and render_person_doc / render_import_section now go through the same published_field, so the compared value and the published value cannot drift apart.

The rest of the delta

  • Scope. 72c21594..9c437622 touches 5 files: finalize.py, its two test files, SKILL.md, CHANGELOG-PENDING.md. Nothing outside the skill except the changelog entry. No new network call, no new sink, no change to the --forget confinement or the redaction path I cleared earlier.
  • No regression in the full-commit path. set(chosen) == set(pending) still publishes staged/people.json verbatim; only the subset branch changed. people_candidates(cap=None) is new[:None], i.e. everyone — used only for the re-cut of an already reviewed set, never for a digest.
  • Docs match the behavior. SKILL.md steps 8/10 and the flag table now say a subset publishes only staged people cut to the landed citations, and that a project's own view needs --stage --projects <slug> and that digest's yes. That is the right escape hatch for the capped-out person, and it keeps consent tied to a digest.
  • Suites at this head, run here: common 10, extract 17, finalize 38, index 21, progress 3, prompts 3, stage 51 (143 importer), plus context-resume 17 and session-recap-extract 13 — 173 total, all OK.
  • Added-line scan. bash scripts/review-checks.sh --diff <cumulative PR diff> run from a checkout at this head (so prose-cap has a post-image and is not silently SKIPPED): PASS (hardcoded-paths + root-artifacts + prose-cap clean). A synthetic /Users/<name>/… control is rejected by the same runner. By hand, every /Users/... added line is a synthetic /Users/o/... test fixture.

Gates, stated so nothing is over-read

CI at this head is green: 20/20 checks pass, including the two that actually exercise this diff — python standalone tests (7m15s) and diff coverage >= 95% (python) (10m25s) — plus license/cla. For the record, tsc + tests (clean install) first reported failure at 3s with its only failing step Require every test job to have succeeded: that run started 19s after the push, before the test jobs finished, so it was the aggregator racing rather than a finding; it now passes. Head unchanged at 9c437622 when this review was submitted; mergeable: MERGEABLE.

Other reviewers' current decisive state: marklysze APPROVED at e0f79d9e (stale — content landed after it), yixuan-ag2 DISMISSED at e212391b. Neither is a standing block; no keweichen review exists on this PR. So after this review nothing from my login blocks, and the two-maintainer approval count still needs a fresh approval on a current head from someone other than me.

This review does not waive required CI, CLA, or the two-maintainer gate, and is not a merge. I verified the two blockers above and scanned the whole 72c21594..9c437622 delta; I did not re-derive the earlier redaction, --forget confinement or known-people findings at this head beyond confirming those files are untouched by these commits. No merge, no contributor-branch edit, no live owner data touched — synthetic fixtures and temporary directories only.

— Sutando core (qingyun-001)

@qingyun-wu

Copy link
Copy Markdown
Collaborator

Merge hold from this seat, for about an hour. The APPROVE at 11:46Z from this login verified the two blockers of the earlier ac96a5c6 review and is correct on those. It did not evaluate the finding posted at 11:44Z by the Codex review under the same login — the revocation path (finalize.py ~1411-1414): refresh_people_export re-ranks all stored entities after --forget-session / --forget, so a never-published person ("Zed Hidden" in the 25-Alpha + 1-Beta fixture) enters the export. That finding carries a control (--stage --projects beta; --commit adds no unseen name) and reproduces at ac96a5c6, so it is not a regression from 9c437622, and 4d9e4e3f9 is a main merge with no content change.

I am re-running that exact probe against 4d9e4e3f9 now. If it reproduces, this seat re-blocks once with the tails and the one-line fix (restrict the post-revocation export to previously-published minus revoked, and test membership, not count); if it does not, the APPROVE stands and I say so here. One decision, not another round — please do not merge on the two approvals until this comment is followed up.

— Sutando core (qingyun-001)

@qingyun-wu qingyun-wu 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.

Changes requested at current head 4d9e4e3f9f3a24380f14b758613f21a621795897.

The main merge leaves every topic file unchanged. I reran the production revocation probe and all 173 importer/adjacent tests on this head. The suites pass, but the new revocation finding remains:

--stage; --commit: reviewed/exported = 25 Alpha names; Zed Hidden absent
--forget-session <Alpha session>: export = [Zed Hidden], rc=0, people_revoked=24
--forget alpha (fresh fixture): export = [Zed Hidden], rc=0, people_revoked=24
both: staged_remaining=0
CONTROL --stage --projects beta; --commit: Zed Hidden shown before publication

skills/import-claude-context/scripts/finalize.py:1413 still re-ranks the stored entities during deletion. The blocker is the unseen identity entering the approved export without another reviewed commit, as documented in #4127 (review). Apply the shared reviewed-set restriction in refresh_people_export and retain membership-based deletion tests. This mitigates the risk of publishing an unreviewed contact while the owner is removing information.

The earlier partial-commit and field-preview findings remain resolved; this does not reopen them. The intervening approval from this shared login evaluated those older findings and did not establish that revocation was fixed, so the formal state must continue to request changes for this independently repeated blocker. Current topic scan passes with the previously verified rejecting control. No additional inline duplicate or contributor-branch change.

Reviewed by Qingyun’s Codex automation.

bassilkhilo-ag2 and others added 2 commits September 11, 2026 15:52
… set and never re-ranks the inputs

`refresh_people_export` — what `--forget`, `--forget-session` and `--hold`
call to cut the approved People export down to the citations still approved
— recomputed the export from `approved/people-inputs.json` with
`people_payloads()` and the default cap. That file holds every approved
person, the cap included the ones the digest never showed; so once the
revoked citations were gone and the people they carried fell under the
floor, the cap opened and the re-ranking filled the freed slots with people
nobody had approved.

The reviewer's replay (PR #4127): 25 Alpha-only people plus one Beta-only
person, two citations each, the Alpha names sorting first. `--stage` then
`--commit` publishes the 25 Alpha names and hides "Zed Hidden" behind the
cap, as designed; `--forget-session <first Alpha session>` (or `--forget
alpha`) then returned rc=0 with the export = ["Zed Hidden"] and
`people_revoked=24`. A removal request had added a person, straight past
the reviewed-set guard (`within_reviewed`) that 9c43762 put on the
`--commit --projects` path.

Fix: the shrink starts from `people.json` — the published set — and goes
through `reviewed_subset_payloads`, the same helper the subset commit uses:
every published person re-rendered from the approved inputs minus the
revoked citations (`cap=None`; the cap already applied when the set was
cut), kept only while they still clear the floor, in the published order.
Nobody is added, no citation is added, nothing is re-ranked;
`people_revoked` is the number of removals. The helper's docstring now
names both callers.

Tests (tests/import-claude-context-stage.test.py, on the 25+1 fixture, each
failing at 9c43762 with "Zed Hidden" in the export and passing now):
- test_forget_session_never_surfaces_a_person_the_cap_hid — five of the
  Alpha people carry a second U2 citation: after `--forget-session U1` the
  export is exactly those five (in the published order, U1 gone from every
  dossier, U2 still cited), `people_revoked=20`, Zed absent; a second
  `--forget-session U2` empties it (`people_revoked=5`).
- test_forget_a_project_never_surfaces_a_person_the_cap_hid — `--forget
  alpha` leaves Beta landed and the export empty (`people_revoked=25`),
  never [Zed].
- test_hold_never_surfaces_a_person_the_cap_hid — the same through `--hold`
  (the session's state record rewritten without `summarized_at`, so the
  hold is not refused as landed).
Each also checks that `--people-json` prints exactly the export and that
`--people-json --projects beta` stays empty. The existing revocation tests
(the Ada store match kept through `--forget beta`, `--forget-session` of a
landed session emptying the export) are unchanged and green.

SKILL.md (the never-do list and the files table) and CHANGELOG-PENDING.md
say that a removal starts from the published set and never surfaces a
person the cap hid.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

@qingyun-wu the revocation path is closed at 4492e86a (head 0ee80e27 = that + main merged, 0 behind): refresh_people_export now starts from the published people.json and goes through the same reviewed-set helper as a subset commit — revoked citations dropped, people under the floor removed, nothing added, no re-ranking; people_revoked counts removals only. Your 25+1 replay is pinned as membership tests for --forget-session, --forget alpha and --hold (all fail on 9c43762, pass now; the cap-hidden person never appears; --people-json prints exactly the export). The clean-install job you saw red at 9c43762 was a concurrency-cancelled duplicate run; the superseding run on the same SHA passed. Gates PASS, diff coverage 100%, stage suite 54.

@john-the-dev john-the-dev 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.

I missed a live hole in the boundary I said I had checked. Owning that first, then the verification.

My approval said I reviewed "the consent boundary: finalize.py's selection and diff paths, their call graph." At the sha I actually read (9c4376226), the revocation path was not behind the guard:

functions calling people_payloads @9c4376226   within_reviewed?
  reviewed_subset_payloads                      yes
  refresh_people_export                         NO      <- @qingyun-wu's finding
  stage                                         no  (correct — staging is where the cap applies)
  main                                          no  (CLI dispatch)

Why my method could not have found it. I enumerated the guard's call sites — within_reviewed, 2 sites, both intersecting — and concluded it was structural. That is a forward check: it proves everyone who calls the guard uses it correctly. It says nothing about who selects people without calling it. The question that finds this is the inverse one — "which paths choose people and do not go through the guard?" — and it is one command. I have written up "check the unguarded sibling branch" as a lesson before; I ran the version of it that confirms rather than the version that falsifies.

That the guard already existed and one path simply did not use it is the part worth keeping. A guard's presence and its coverage are different facts, and I reported the first as if it were the second.

The finding is real, and the fix landed 76 seconds after it was filed

qingyun-wu CHANGES_REQUESTED  11:50:44Z  at 4d9e4e3f9
4492e86aa  "fix(import-claude-context): a revocation shrinks the published…"  11:52:00Z

So their block was accurate at their sha and is now stale rather than wrong. I checked that rather than assuming either way.

Verified at the current head

refresh_people_export now passes published (people.json — the set already published) through reviewed_subset_payloads into the same within_reviewed, so the revocation path and the subset-commit path share one guard. Reproduced their probe:

within_reviewed(published=25 Alpha names, pool incl. Zed Hidden) -> ['Alpha 00', 'Alpha 07']

  'Zed Hidden' enters the export?          False    <- the hidden identity stays hidden
  CONTROL survivors still exported          True
  CONTROL a shrink can still remove         25 published -> 2 exported
  VACUITY published=[] -> []                publishes nobody

The third control matters as much as the first: a guard that stopped the export shrinking would break revocation itself, trading an over-share for a failure to forget. Removal still removes.

This was the worst shape the original defect could take. An unreviewed contact entering the export during a commit is bad; entering it while the owner is running --forget is the same data crossing the same boundary at the moment they acted to reduce what is held.

Re-approving at the current head

Two process notes, since both bit me here:

  • My recorded review reads APPROVED at 4d9e4e3f9 — GitHub stamped it forward. The content I read was 9c4376226. A review's commit_id is not evidence of what the reviewer saw, in either direction.
  • I told @bassilkhilo-ag2 I would merge this "as soon as the checks go green." That was wrong the moment a live block existed, and I would have been merging over a real consent defect had the author not already fixed it. Correcting it there too.

@qingyun-wu qingyun-wu 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.

Approved at 0ee80e27754ae8253b628f0ee02f0868a2503eec. This replaces this login's CHANGES_REQUESTED and closes the hold from my earlier comment.

The one open item — the revocation path re-ranking the stored entities — is fixed in 4492e86aa and verified here with a control, run in a fresh clone:

tests/import-claude-context-stage.test.py @ 0ee80e277:              Ran 54 tests — OK
same tests with finalize.py from 9c437622 (the previously blocked head):
  FAIL test_forget_session_never_surfaces_a_person_the_cap_hid
  FAIL test_forget_a_project_never_surfaces_a_person_the_cap_hid
  FAIL test_hold_never_surfaces_a_person_the_cap_hid
  Ran 54 — FAILED (failures=3)

Exactly the three revocation cases fail on the old finalizer and nothing else moves, so the tests discriminate the fix rather than the vocabulary. The two earlier findings (partial commit inside the reviewed set; same-prefix role changes in the digest) stay resolved. CI at head still has pending jobs (diff coverage >= 95% (python)=pending python standalone tests=pending typecheck + node & shell tests=pending ) — the merge waits on them, the review does not.

— Sutando core (qingyun-001)

…ore flow

Mirror of cinny#958's fix for john-the-dev's review: the disclosure was
silent about people named in the transcripts becoming records in the hosted
People store after the owner approves the digest. The acknowledgement,
completion message, description and docs now say so.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XruNJUKemwtNbpHsLkebzf
@github-actions

Copy link
Copy Markdown
Contributor

@cla-assistant check

@qingyun-wu qingyun-wu 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.

LGTM, non-blocking — approving 67c64062271a3299ce7f4a24fa6e7a146462acc2. The revocation finding from my previous review is fixed; this also checks the later People-store disclosure change. Current CI is still running, so this is not a claim that all merge gates have completed.

I compared the complete 20-file PR inventory with the last examined head 4d9e4e3f9: 15 files are byte-identical; re-read the five changed files and traced stage, full/subset commit, all three revocation callers, export rendering and identity matching. Revocation now uses the same reviewed-set helper as subset commit, with published people.json as its boundary. The revised consent sentence correctly names the approved People-store upload.

Independent replay of the original fixture through production CLI dispatch (temporary workspace; only the memory-budget subprocess stubbed):

                                      4d9e4e3f9           67c640622
full stage/commit: shown/published     25 / 25              25 / 25
--forget-session: newly published      ['Zed Hidden']      []
--forget alpha: newly published        ['Zed Hidden']      []
people_revoked, both removal paths     24                  25
explicit beta-only stage/commit        shown then saved    shown then saved

The same current 54-test stage suite with the previous finalizer fails exactly the three new forget-session/forget/hold membership tests; restoring HEAD passes 54/54. All nine importer/adjacent suites pass: 176 tests. The new survivor cases also retain five approved people after the first removal, then remove them after their remaining citations are revoked, so an always-empty export would not pass.

Worst-case disruption was publishing an unseen contact while the owner was asking to forget information. The fixed boundary excludes hidden candidates while preserving removal, survivor order and explicit re-review. The previous live import evidence was not independently repeated; these are isolated CLI and regression checks, with no external People writes. Full-diff path/root/prose scans pass; an injected forbidden host-path control is rejected. CLA is green; normal current-head CI and maintainer approval requirements still apply. No merge or contributor-branch modification.

Reviewed by Qingyun’s Codex automation.

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.

5 participants