Repository navigation
Conversation
|
Important Approval pendingCodeRabbit has no unresolved comments, but it has not reviewed the latest commit. Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #410 - fix: redact password fragments around PEM placeholders
Head: b452129ef36593e3b3e9da9f19095acbdedab5da into main. Review event: COMMENT.
Estimated review effort: 2/5 (~24 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
ingest_protection.py |
modified | +19/-6 | Changed file | Elevated: validated P1 finding |
tests/test_embedding_privacy.py |
modified | +41/-6 | Test coverage | Low |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 1, P2: 0, P3: 0).
Dropped findings before posting: 0. High-severity findings: 1.
Maintainer Analysis
Changed behavior:
- Embedding privacy transform identity advances from privacy:v3 to privacy:v4.
- Password-assignment matching treats an emitted sensitive placeholder as one quoted or unquoted value atom so matching can cross its closing bracket.
Affected invariants:
- Provider-bound embedding text must not retain password fragments surrounding redacted PEM material.
- Transform behavior changes must use a distinct revision so older vectors become pending.
Evidence:
- ingest_protection.py:403 defines the sensitive-placeholder span.
- ingest_protection.py:434-438 applies a six-atom minimum while each complete placeholder counts as one atom.
- tests/test_embedding_privacy.py:640 adds coverage only with long
prefixSECRETandsuffixSECRETfragments, leaving the failing short-fragment boundary uncovered.
Limitations:
- Review was limited to the supplied checkout diff; no shell commands, project tests, builds, or arbitrary PR code were run.
No-finding rationale: No additional correctness, data-loss, CI, compatibility, or regression defect was validated from the supplied diff.
Risk Taxonomy
- Security boundary: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #372.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
There was a problem hiding this comment.
Walkthrough
PR: #410 - fix: redact password fragments around PEM placeholders
Head: 3f101fdcd9afaa2b9b8334765493c32b26807149 into main. Review event: COMMENT.
Estimated review effort: 3/5 (~34 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
ingest_protection.py |
modified | +91/-7 | Changed file | Elevated: validated P1 finding |
tests/test_embedding_privacy.py |
modified | +88/-7 | Test coverage | Low |
Review Signal
Validated inline findings: 3 (P0: 0, P1: 2, P2: 1, P3: 0).
Dropped findings before posting: 0. High-severity findings: 2.
Maintainer Analysis
Changed behavior:
- Embedding privacy identity advances from privacy:v3 to privacy:v4, making prior vectors pending for re-embedding.
- Password assignments containing an already-emitted private-key placeholder are collapsed into a password-assignment placeholder.
- Sensitive placeholders are treated as single atoms by password-assignment regexes.
Affected invariants:
- Password fragments surrounding PEM material must not survive durable or provider-copy redaction.
- Durable redaction must not consume unrelated context across assignment boundaries.
- Sensitive-text scanning must remain linear on pathological input.
Evidence:
- ingest_protection.py:404 placeholder-span pattern
- ingest_protection.py:526 placeholder recognition inside the value scanner
- ingest_protection.py:533 quoted-value termination
- tests/test_embedding_privacy.py adds only alphanumeric prefix/suffix composition cases
Limitations:
- Review was limited to the supplied checkout diff; no commands, tests, builds, or package metadata were executed or inspected.
No-finding rationale: The remaining changes are consistent with transform-version invalidation and the intended placeholder composition; no additional defect was validated from the supplied diff.
Risk Taxonomy
- Data loss: 1
- Runtime correctness: 1
- Security boundary: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #372.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
There was a problem hiding this comment.
Walkthrough
PR: #410 - fix: redact password fragments around PEM placeholders
Head: 421e92e9b98b73dfa6506833b921f9921ddcee52 into main. Review event: COMMENT.
Estimated review effort: 3/5 (~34 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
ingest_protection.py |
modified | +132/-16 | Changed file | Elevated: validated P1 finding |
tests/test_embedding_privacy.py |
modified | +165/-7 | Test coverage | Low |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 1, P2: 0, P3: 0).
Dropped findings before posting: 0. High-severity findings: 1.
Maintainer Analysis
Changed behavior:
- Embedding privacy identity advances from privacy:v3 to privacy:v4.
- Password assignments containing canonical private-key placeholders are replaced as a whole, including short fragments around the placeholder.
- LCM placeholder recognition is restricted to canonical names and metadata shapes.
Affected invariants:
- Provider-bound embedding text must not retain password fragments around a private-key placeholder.
- Durable redaction must remove the same sensitive fragments without consuming unrelated text.
- Privacy-transform generations must use distinct vector identities.
Evidence:
- ingest_protection.py:567 skips the character immediately after a backslash during quoted-value scanning.
- tests/test_embedding_privacy.py adds short-fragment, delimiter, escaped-quote, and linearity coverage, but no case with a backslash immediately before the PEM placeholder.
Limitations:
- Review was limited to the supplied diff and checkout context; no code, tests, builds, or shell commands were executed.
No-finding rationale: No additional correctness, data-loss, CI, or compatibility defect was validated from the supplied diff.
Risk Taxonomy
- Security boundary: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #372.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
There was a problem hiding this comment.
Walkthrough
PR: #410 - fix: redact password fragments around PEM placeholders
Head: 777795e6c9db6de3e67ee0de5be1aab35ecae45c into main. Review event: COMMENT.
Estimated review effort: 3/5 (~34 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
ingest_protection.py |
modified | +139/-16 | Changed file | Elevated: validated P1 finding |
tests/test_embedding_privacy.py |
modified | +191/-7 | Test coverage | Low |
Review Signal
Validated inline findings: 1 (P0: 0, P1: 1, P2: 0, P3: 0).
Dropped findings before posting: 0. High-severity findings: 1.
Maintainer Analysis
Changed behavior:
- Embedding privacy identity advances from privacy:v3 to privacy:v4.
- Canonical PEM placeholders inside password assignments are treated as one atom and cause the surrounding assignment value to be redacted.
- Private-key-placeholder recognition is narrowed to canonical placeholder forms.
Affected invariants:
- Configured sensitive-pattern ordering must not allow password fragments to cross the embedding privacy boundary.
- Durable and provider-bound redaction should remove the same sensitive assignment fragments.
Evidence:
- ingest_protection.py:2064 performs the composition only during the password-assignment iteration.
- ingest_protection.py:610 applies the same order-sensitive strategy to durable redaction.
- The added tests exercise only the default pattern order and do not permute private_key and password_assignment.
Limitations:
- Review was limited to the supplied checkout diff; no commands, tests, builds, or project code were executed.
No-finding rationale: No additional correctness, data-loss, CI, or compatibility defect was validated directly from the supplied diff.
Risk Taxonomy
- Security boundary: 1
Validation and Proof
No required validation recommendation selected; rely on existing GitHub checks and human review.
Proof status: not_applicable - No required behavior proof selected for this changed surface.
Related Context
Related issues/PRs: #372.
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
|
Disposition: accepted for the v0.24.0 train (not the point release), review starting now. Context for the routing: cloud-embedding PEM redaction is a frozen best-effort feature on our side (#394 — not release-blocking, exposure-weighted), so I would not have spent maintainer time hardening it, but a finished contribution closing #372 is a different matter and gets a proper review. Two consequences of the An independent adversarial review lane (Claude — our codex reviewer cannot read PEM material) is running on exact head |
100yenadmin
left a comment
100yenadmin
left a comment
There was a problem hiding this comment.
Independent adversarial review (Opus deep-reasoner lane, exact head 777795e6) — FAIL, 4 findings; two P1s reproduced first-hand by the maintainer. Full report: session-notes review-410-opus-deep-reasoner.md; my repro transcript probe-410/REPRO-410-firsthand.txt.
The declared fix holds. Across every separator shape the reviewer tried (:, =, JSON-quoted, single-quoted, unquoted, bracketed) the base leaks both PEM fragments inside a password value and head leaks none, durable and provider; escaped-quote parity 0–16 exhaustive, zero leaks; linearity confirmed to n=16000. privacy:v4 is single-sourced and folded into identity_hash, both entry points refuse on mismatch. Tests: test_ingest_protection.py 144 passed / 1 skipped (pre-existing #408 skip), test_embedding_privacy.py 94 passed, neighbours green.
But the new password_assignment value regex matches its own emitted placeholder, and that breaks ordinary clean text:
- P1 — residual detector fails closed on non-secret documents (recall regression on the default cloud posture).
{"password": "hunter2000", "user": "bob"}— no PEM anywhere — HEAD raisesEmbeddingPrivacyPolicyError: … residual detector blocked pattern names: password_assignment; BASE returns{"password": "[LCM embedding privacy: name=password_assignment]", "user": "bob"}. Root:_PASSWORD_ASSIGNMENT_QUOTED_ATOM([^\r\n\]\}]) does not exclude the active quote, so with the placeholder-as-atom alternative the lazy{6,}?swallows the placeholder and runs past the true closing quote to the next quoted token on the line. Any single-line JSON / env / tool-arg text with a password plus one later quoted token trips it. Consequence:command.py:3821-3833counts the documentblockedand silently skips it from embedding;tools.py:3762/engine.py:6716disable recall. (ingest_protection.py:427-429,:457-464, detector:2034.) - P1 — same root cause on the durable side: non-secret content destroyed,
redact_sensitive_textno longer idempotent.password="a<PEM>" and note="keep"→ durablepassword="[LCM sensitive redaction: name=password_assignment; chars=82; bytes=82]"keep"—and note=is gone. Re-applying redaction to the JSON case yields{"password": "[...chars=79...]": "bob"}. Base: idempotent on all four probe cases; head: two of four. - P2 — the #391 proximity backstop was narrowed without disclosure.
ingest_protection.py:1978swaps the permissive placeholder regex for the canonical-only one; six non-canonicalprivate_keyplaceholder + 26-char base64 probes: BASE blocks 6/6, HEAD blocks 3/6 (name=private_key; note=x],name=private_key ], newline-after-colon reach the provider). If intentional, say so in the body and add a test pinning the new trigger set; otherwise keep the permissive regex there. - P2 — over-redaction on an unterminated quote.
password: 'twas rotated, see key <PEM> in the vault for details→ both paths destroyin the vault for details; BASE preserves it. The manual scanner (:551-576) lacks the “no closing quote → abandon” fallback the regex gets from(?P=quote).
Validated fix for 1+2 (reviewer-tested in a scratch copy of head): make the quoted atom exclude the active quote —
_PASSWORD_ASSIGNMENT_QUOTED_ATOM = r"(?:(?:" + _SENSITIVE_PLACEHOLDER_SPAN_PATTERN + r")|(?!(?P=quote))[^\r\n\]\}])" — keeps 238 passed / 1 skipped, takes durable-vs-provider divergences 1 → 0 across the reviewer's 15-case corpus, restores idempotency, and keeps every #372 shape closed. Please add the two P1 probes above as regression tests (they are the cheapest possible guards for this class), plus an idempotency assertion on redact_sensitive_text.
Also needed before merge: a CHANGELOG / release-notes line telling cloud-embedding users that privacy:v4 forces a one-time re-embed (/lcm embed warmup + backfill).
Disposition + confidence (owner delegation rule, recorded): findings 1–2 MERGE_BLOCKING · FIX NOW; 3–4 MERGE_BLOCKING pending disclosure/fix. The v0.24.0 acceptance stands (batched identity bump with #188, ahead of the F53 re-bank) — conditional on the revision. This PR changes the embedding transform, so it sits under the ≥95% bar: the revised head gets a fresh exact-head adversarial lane before receipts; I will not merge on today's review.
|
Thanks @Tosko4 for the careful analysis here. We're closing this PR because it targets a base that is now 267 commits behind main (v0.24.0 shipped the hermes-lcm-x rename and fix train, and v0.24.1 followed with #510/#515/#518). The privacy:v4 transform bump would also force a new release and a fleet-wide re-embed, which we're not opening right now without a verified P0. Issue #372 stays open to track the gap. If it's still relevant, please refile a rebased PR against current main and we'll look at it in the next planned release window. |
2 participants
Summary
privacy:v4so old and new vector generations cannot mix.Linked work and change class
2e17bbc89f05f0edd01aa9e459915be231e76181headRefOid):777795e6c9db6de3e67ee0de5be1aab35ecae45cWhy
The private-key-first transform correctly protects key material, but the emitted placeholder contains
]. The password-assignment pattern stopped at that delimiter, leaving secret fragments before and after the placeholder. In quoted assignments, an odd backslash immediately before the placeholder start also caused the scanner to skip the opening[and miss the already-redacted private key entirely.Root cause and best-fix assessment
ingest_protection.pyowns both transforms.Hermes compatibility and lossless-data impact
privacy:v4changes the vector profile identity, so affected installations must complete the normal embedding warmup/re-embed path before relying on prior semantic coverage.Real-behavior proof
237 passed, 1 skipped, 1 failedacrosstests/test_embedding_privacy.py tests/test_ingest_protection.py. The sole failure is the unchanged three-second unmatched-marker timing assertion; it also failed on exactmainat 3.11 seconds in the same environment, while an isolated current-head run completed at 2.98 seconds. The changed password-assignment helper is not reached by that test.Validation
237 passed, 1 skipped, 1 failed; the sole timing failure reproduced on exactmainin the same environment.ruff check ingest_protection.py tests/test_embedding_privacy.pypassed.python3 -m py_compile ingest_protection.py tests/test_embedding_privacy.pypassed.git diff --checkpassed.Release-note impact
Review and merge gate
headRefOidis recorded.AI review exact-headpassed on the current head.Non-goals and notes