Release 0.0.38: PII masking, next/previous fixes, how display fixes, behavior indicator#365
Release 0.0.38: PII masking, next/previous fixes, how display fixes, behavior indicator#365jbutcher21 merged 11 commits intomainfrom
Conversation
🤖 Claude Code ReviewNow I have enough information to write the review. PR Code Review: v0.0.38Summary: This PR adds Code Quality✅ No commented-out code — The diff actually removes previously commented-out code (e.g., ✅ Meaningful variable names — ✅ DRY principle — PII masking logic is centralized in ❌ Minor style issue — argparse flag uses underscore instead of hyphen argParser.add_argument(
"-M",
"--mask-pii", # <-- hyphen, not underscore
...
)❌ Potential defect — ❌ Minor defect — token not in ',;:|/\\-'This works for single-char separators (substring check on a 7-char string), but is semantically misleading — it reads as substring containment, not set membership. Should use: token not in {',', ';', ':', '|', '/', '\\', '-'}✅ No logic errors in ✅ ROLE-based fix for columnar view (#341) — The switch from checking ✅ ✅ Testing❌ No unit tests for new pure functions — ❌ No tests for ❌ No existing Documentation✅ CHANGELOG.md updated — All four fixes and two features are listed with issue references. ✅ Version bumped — ✅ CLI help text provided — The ✅ No inline comments needed — The PII masking functions have docstrings. Security✅ No hardcoded credentials ✅ No license files ( ✅ PII masking is additive/opt-in — The default is
✅ No SQL injection risk — The Summary
Key items to address before merge:
Automated code review analyzing defects and coding standards |
🤖 Claude Code ReviewNow I have enough context for a full review. Here's my analysis: PR Code Review: v0.0.38Code Quality✅ No commented-out code — The diff also removes previously commented-out lines (e.g., ✅ Meaningful variable names — ✅ DRY principle — ❌ Line length — sql = sz_dbo.sql_prep(f"select RES_ENT_ID from RES_ENT where RES_ENT_ID {operator} ? order by RES_ENT_ID {order}")Consider breaking the f-string onto its own line.
✅ ROLE fix is correct — The switch from ✅ Phantom feature ID guard — ✅ No SQL injection risk — Testing❌ No tests for new functionality —
These are particularly important for ❌ Test coverage — Coverage cannot exceed the placeholder test. This should be addressed. Documentation✅ CHANGELOG.md updated — Version 0.0.38 entry is correctly placed with added features and fixes, dated ✅ ✅ Inline docstrings present on
Security✅ No hardcoded credentials. ✅ No sensitive data in logs — The warning at ✅ No license files (
Summary
Blocking items: The lack of unit tests for
|
🤖 Claude Code ReviewNow I have enough context to provide a thorough review. PR Code Review: v0.0.38Code Quality✅ No commented-out code — The PR actually removes previously commented-out dead code ( ✅ Meaningful variable names — ✅ DRY principle — PII masking is centralized in ❌ Line length violations — The project enforces 120-char lines via Black, but several new lines exceed this:
❌ Input validation missing for # In argparser:
type=int,
metavar="N",
# Or add a validation after parse_args:
if args.mask_pii is not None and args.mask_pii < 0:
argParser.error("--mask_pii must be >= 0")✅ No defects found in core logic — The ✅ Behavior rank logic — ℹ️ Minor: ℹ️ Minor: Testing❌ No unit tests for new functions —
❌ Test coverage — No coverage measurement can currently verify the >80% threshold with only the example test. Documentation✅ CHANGELOG.md updated — Correct Keep a Changelog format with ✅ pyproject.toml version bumped — ✅ Inline comments — New sections use the existing Security✅ No hardcoded credentials ✅ No sensitive data in logs — The warning message at ✅ No license files ( ✅ SQL parameterization — The new SQL in ✅ PII masking applied before caching — Summary
The functional changes (PII masking, behavior indicator,
|
🤖 Claude Code ReviewNow I have enough context. Here is the full review: Code Review:
|
| Severity | Issue |
|---|---|
| High | behavior_rank incorrectly handles "NAME" frequency — wrong arrows on name features |
| Medium | Line length violations on 2 lines |
| Medium | mask_json_pii mutates in-place before caching — debug cache contains masked data |
| Low | -M 0 silently disables masking instead of masking all characters |
| Low | No unit tests for pure functions (mask_pii, mask_json_pii, behavior_rank) |
Automated code review analyzing defects and coding standards
jbutcher21
force-pushed
the
363-jb-mask-pii
branch
from
6d55ef3 to
3c8c8e8
Compare
🤖 Claude Code ReviewNow I have enough context for a thorough review. Code Review: PR to v0.0.38Code Quality✅ No commented-out code — The diff actually removes two blocks of commented-out code (good cleanup). ✅ Meaningful variable names — ✅ DRY principle — PII masking logic is centralized in ❌ Line length violation — sdk_wrapper = EdaSdkWrapper(engine_config, debug_trace=args.debug_trace, webapp_url=args.webapp_url, mask_pii=args.mask_pii)Should be wrapped (Black would split this across lines). ❌ Potential defect — matched_desc = f"\u2514\u2500\u2500 {feature['MATCHED_FEAT_DESC']} ({score}){behavior_change}"With ❌ Edge case in if not score_behavior or not expected or score_behavior == expected:
return ""❌ Performance concern in SELECT r.RES_ENT_ID FROM RES_ENT r
JOIN RES_ENT_OKEY o ON r.RES_ENT_ID = o.RES_ENT_ID
WHERE r.RES_ENT_ID {operator} ? ORDER BY r.RES_ENT_ID {order} LIMIT 1
Testing❌ No unit tests for new functions —
Current test coverage is essentially zero for this feature. Only Documentation✅ CHANGELOG updated — Correctly documents what changed in 0.0.38. ✅ Version bumped — ✅ Inline docstrings — Security✅ No hardcoded credentials. ✅ SQL parameterization — ✅ No sensitive data in logs — The ✅ No license files (.lic) committed. Summary
Automated code review analyzing defects and coding standards |
🤖 Claude Code ReviewI'll proceed with the review based on what's visible in the diff and the project's CLAUDE.md. Code Review: PR to v0.0.38Code QualityCode Style ✅ No Commented-Out Code ✅ Meaningful Variable Names ✅ DRY Principle ✅ Defects / Logic Issues ❌ (minor)
CLAUDE.md ✅ Testing ❌No test files are added or modified in this PR. New logic introduced includes:
These are all independently testable pure/near-pure functions. DocumentationCHANGELOG.md ✅ — Updated with clear entries for all changes. Version bump ✅ — README — Cannot verify from the diff if Inline comments ✅ — Markdown formatting ✅ — CHANGELOG additions follow the existing format. SecurityNo hardcoded credentials ✅ PII masking feature ✅ — Correctly applied at the API response level ( Warning message exposes identifiers — The new warning at print_message(f"Feature ID {lib_feat_id} on record {record['DATA_SOURCE']}: {record['RECORD_ID']} not in entity features", "warning")This logs No Input validation — See defect #1 above (negative Summary
Must fix: The
|
2 participants
Summary
--mask_pii(-M) CLI flag to sz_explorer for PII data masking with configurable visible characters (-Mdefaults to 2,-M1for 1,-M3for 3)INBOUND_VIRTUAL_ENTITY_IDfrom SDK response instead of assuming VIRTUAL_ENTITY_1 is always inboundTest plan
sz_explorer -M— verify masking with 2 visible charssz_explorer -M1— verify masking with 1 visible charshow_last_call— verify raw JSON is also maskedwhy <id1> <id2>— verify ↓/↑ indicator appears when scoring behavior differs from confighow <entity_id>columnar view — verify features align with correct virtual entity columnshow <entity_id>on entity with 3+ records — verify INBOUND/CANDIDATE sides are correctget <id>thennext/previous— verify correct navigationpreviouswith no priorget— verify graceful warning, no crashnext/previouswithout direct database access — verify graceful warninghowon entity 23 (Maria Sentosa) with SDK 4.2.0 — verify warning instead of crash for phantom feature IDsResolves #363
Resolves #364
Resolves #341
Resolves #340