Address follow-up review feedback

[yimsk]

Summary

• Refine AI raw-data redaction to avoid broad environment/variables false positives while still redacting sensitive key names and values.
• Handle acronym-prefixed sensitive keys such as DBPassword and DBMasterPassword.
• Simplify S3/IAM enrichment status rendering and pagination token filtering after PR Harden AWS resource rendering and demo workflow #180 review feedback.

Validation

• go test ./internal/ai ./internal/view ./custom/s3/buckets ./custom/iam/policies
• go test ./...
• go vet ./...
• go build ./cmd/claws
• golangci-lint run --config .golangci.yml ./...
• gofmt -l cmd internal custom scripts
• git diff --check

Context

Follow-up for the post-merge Claude review on #180.

[claude]

Code Review

Overall this is a well-structured follow-up PR that addresses real issues from #180. The changes are focused and mostly improvements. One behavior change in the redaction logic deserves discussion.

---

custom/iam/policies/dao.go ✅

Dead code removal is correct. The else { res.PolicyDocumentStatus = enrichment.Fetched } branch was only reachable when err == nil, but the if arm above already covered that case. Clean.

---

custom/s3/buckets/render.go ✅

Replacing the triple-OR condition with enrichment.IsFailure() is a nice readability improvement and makes the code more resilient if new failure statuses are added to the enrichment package later. Good abstraction.

---

internal/ai/session.go + session_test.go ✅

Extracting sessionPath() is the right call — the path construction logic was duplicated in LoadSession, saveSession, and the test. The test using the helper directly is strictly better than hardcoding the expected path string.

---

internal/view/resource_browser_fetch.go ✅

hasProfileRegionToken is a minor but appreciated clarity win. The flattening of the nested if reads more naturally. No behavioral change.

---

internal/ai/tools.go — Needs Discussion

redactSensitiveRaw fast path ✅
The optimization to skip the JSON round-trip when the value is already map[string]any or []any is sound. The comment explains the intent clearly.

[]map[string]any case in redactSensitiveValue ✅
Correct fix. Without this case, a typed []map[string]any would fall through to []any only if the interface conversion happens to work — adding the explicit case is safer.

shouldSplitRawKeySegment / acronym handling ✅
The logic is correct. The algorithm properly identifies the B→P boundary in DBPassword (uppercase followed by uppercase-then-lowercase signals the start of a new word). Traced through manually:

• DBPassword → ["db", "password"] → "password" matches → sensitive ✓
• DBMasterPassword → ["db", "master", "password"] → sensitive ✓
• environment → ["environment"] → no match → not sensitive ✓

Removing "environment" / "variables" from sensitive sets ✅
The motivation is clear and the fix is targeted. EnvironmentVariables still matches via "environmentvariables" in the exact set, and DB_PASSWORD / DBPassword both now correctly match through the improved segmenter.

Key-name redaction: redacted["[REDACTED]"] = "[REDACTED]"] ⚠️

This is the one change worth reconsidering. Previously:

redacted[key] = "[REDACTED]"   // preserved key name, hid value

Now:

redacted["[REDACTED]"] = "[REDACTED]"   // hides both key name and value

Two concerns:

1. Map key collision: If a resource has multiple sensitive fields (e.g. DBPassword, ApiToken, clientSecret), all three overwrite the same "[REDACTED]" key. The output map ends up with a single entry instead of three. The AI (and any human reader) loses the information that multiple sensitive fields existed.

2. Key names are rarely sensitive: Field names like DBPassword or ApiToken describe structure, not secrets. Preserving the key name while redacting the value ({"DBPassword": "[REDACTED]"}) gives the AI useful schema context without exposing the actual secret. The new form ({"[REDACTED]": "[REDACTED]"}) degrades AI usefulness without a meaningful security gain.

Suggestion: revert this specific change back to redacted[key] = "[REDACTED]" — the key name redaction solves a problem that wasn't there.

---

Test Coverage ✅

• TestIsSensitiveRawKeyAvoidsSubstringFalsePositives now covers both the newly-safe (environment, variables) and newly-sensitive (DBPassword, DBMasterPassword, MasterUserPassword) keys.
• TestFormatResourceDetailKeepsNonSensitiveEnvironmentFields is a well-targeted regression test for the false-positive fix.
• Using sm.sessionPath() in the session persistence test is an improvement over the hardcoded path string.

---

Summary

Change	Verdict
IAM dead code removal	✅ Merge
S3 IsFailure() refactor	✅ Merge
sessionPath() extraction	✅ Merge
hasProfileRegionToken() extraction	✅ Merge
redactSensitiveRaw fast path	✅ Merge
Acronym segment splitting	✅ Merge
Remove environment/variables from sensitive sets	✅ Merge
redacted["[REDACTED]"] = "[REDACTED]" key hiding	⚠️ Worth reverting

The key-name redaction is the only change I'd push back on. Everything else is clean, well-tested, and clearly motivated.

[yimsk]

Addressed the latest Claude review note in 9adfd42:

• Reverted sensitive raw-data map handling to preserve key names while redacting values.
• Added regression coverage proving multiple sensitive keys (DBPassword, ApiToken, clientSecret) remain distinct and their values are redacted.
• Updated the persistence test to assert secret values/label values are absent while allowing non-secret schema key names.

Validation run locally:

• go test ./internal/ai
• go test ./...
• go vet ./...
• go build ./cmd/claws
• golangci-lint run --config .golangci.yml ./...
• gofmt -l cmd internal custom scripts
• git diff --check

[claude]

Follow-up Review

The previous concern (map key collision from redacted["[REDACTED]"] = "[REDACTED]") has been addressed in 9adfd42. TestFormatResourceDetailPreservesMultipleSensitiveKeyNames confirms the fix: multiple sensitive keys remain distinct and only their values are redacted. Everything below covers the full diff.

---

custom/iam/policies/dao.go ✅

Dead else branch correctly removed. The Fetched assignment is set on the success path earlier, so the else after if err != nil was unreachable.

---

custom/s3/buckets/render.go ✅

enrichment.IsFailure() is the right abstraction — no behavioral change, but future failure status additions to the enrichment package will be handled automatically.

---

internal/ai/session.go + session_test.go ✅

sessionPath() extraction is clean. Using the helper in the test is strictly better than a hardcoded path string — it stays correct if the path construction ever changes.

---

internal/view/resource_browser_fetch.go ✅

hasProfileRegionToken and the flattened if are a minor readability win with no behavioral change. No issues.

---

internal/ai/tools.go ✅

Fast path in redactSensitiveRaw
Sound optimization. map[string]any and []any skip the JSON round-trip; everything else (typed SDK structs, etc.) still normalizes through JSON. The comment explains the intent clearly.

[]map[string]any case in redactSensitiveValue
This case is necessary specifically for recursive calls: after the fast path, the outer map bypasses JSON normalization, so any nested []map[string]any values would fall to the default branch and escape redaction without this case. Correct fix.

shouldSplitRawKeySegment / acronym splitting
Traced through manually:

Key	Segments	Sensitive?
DBPassword	["db", "password"]	✓
DBMasterPassword	["db", "master", "password"]	✓
APIToken	["api", "token"]	✓
environment	["environment"]	✗ (correct)
ALLCAPS	["allcaps"]	✗ (correct)

The index+1 >= len(runes) guard prevents a panic on the last character. The algorithm correctly uses lookahead for UPPER→UPPER→lower boundaries without being fooled by all-caps sequences.

Removing "environment" / "variables" from sensitive sets
The fix is targeted. EnvironmentVariables still matches via the exact set ("environmentvariables"), and DB_PASSWORD/DBPassword now correctly match through the improved segmenter.

---

session_test.go — TOKEN removal ✅

Removing TOKEN from the "should not contain" assertion is correct given the design decision to preserve key names (redact values only). The key name TOKEN legitimately appears in the persisted file for AI schema context. The test still asserts [REDACTED] is present and the secret values (persist-me-not, persist-output-secret, DB_PASSWORD) are absent.

---

Test Coverage ✅

• TestIsSensitiveRawKeyAvoidsSubstringFalsePositives now covers environment and variables (now-safe) alongside DBPassword, DBMasterPassword, MasterUserPassword (now-sensitive).
• TestFormatResourceDetailKeepsNonSensitiveEnvironmentFields is a well-targeted regression test for the false-positive fix.
• TestFormatResourceDetailPreservesMultipleSensitiveKeyNames directly addresses the previous review concern — multiple distinct sensitive keys remain distinct.

---

Summary

Change	Verdict
IAM dead code removal	✅
S3 IsFailure() abstraction	✅
sessionPath() extraction	✅
hasProfileRegionToken() extraction	✅
redactSensitiveRaw fast path	✅
[]map[string]any recursive case	✅
Acronym segment splitting	✅
Remove environment/variables from sensitive sets	✅
Key name preservation (previous concern resolved)	✅

No remaining issues. Ready to merge.