fix(intelligence): classify env files by evidence not filename (#122)

[SHAURYAKSHARMA24]

Summary

Environment-file review stopped treating a .env-prefixed filename as proof of an exposed secret. The engineering review now classifies each committed environment file by its content and only raises a critical, rotation-worthy finding when a credible secret-like value is actually present.

This is security-sensitive logic (it decides when the product declares a critical secret exposure) and is covered by explicit regression tests. Reviewer attention on the classification and placeholder-detection rules is requested.

Linked issue

Fixes #122

What changed

Backend — app/intelligence

• New EnvironmentFileEvidence model with an evidence_class of template_present, runtime_env_file_present, or secret_like_value_detected, plus the sensitive key names (never values) that triggered detection.
• RepositoryIntelligenceEngine._discovery now derives this evidence by reading each environment file's content. Classification is: content with a non-placeholder secret-like value → secret_like_value_detected; otherwise a .env.example / .env.sample / .env.template / .env.dist name → template_present; otherwise → runtime_env_file_present.
• Placeholder detection treats empty values, ${VAR} / $VAR references, <...> / [...] markers, and common example tokens (example, password, changeme, your-*, …) as non-secrets.
• Embedded-URL credential detection (user:password@host) now inspects the password and ignores it when it is itself a placeholder or a ${VAR} reference — so a sample DATABASE_URL in a template is not a critical finding.
• Credential-key matching now covers common access-key ID, plural credential, and password variants such as AWS_ACCESS_KEY_ID, SERVICE_CREDENTIALS, DB_PASSWD, and DB_PWD; these names still require a credible secret-like value before becoming critical evidence.
• Regression guards pin two important non-secret cases: path-valued GOOGLE_APPLICATION_CREDENTIALS remains configuration evidence, and CLIENT_ID-style keys remain non-critical even when their values look credential-shaped.
• from_record refreshes cached intelligence that predates this evidence, so a stale record cannot resurface a filename-only critical finding.

Backend — app/review

• The single env-file-present (always critical) finding is replaced by up to three evidence-sized findings: env-secret-like-value-detected (critical, security), env-runtime-file-present (medium, security), and env-template-present (low, configuration). Each states its evidence class; only the secret-like finding advises rotation. Cached records without evidence fall back to runtime_env_file_present (never critical).
• Critical findings list at most 10 sensitive key names and append and N more when additional names are omitted, so the report never truncates evidence silently.

Docs

• docs/architecture/REPOSITORY_INTELLIGENCE.md documents the evidence classes and the "filename is never proof, values are never surfaced, rotation advice only on secret-like evidence" behavior.

Integration with latest dev

• Merged the latest dev after feat(intelligence): add versioned snapshot query API #126 landed.
• Resolved the conflicts in app/review/review_service.py and docs/architecture/REPOSITORY_INTELLIGENCE.md by retaining both fix(intelligence): classify env files by evidence not filename (#122) #127's environment-file evidence handling and feat(intelligence): add versioned snapshot query API #126's oversized-source-file evidence behavior.

Root cause and behavior change

Root cause: discovery and the review builder flagged any file whose basename started with .env as a critical secret exposure (Path(path).name.startswith(".env")), with no content analysis. A filename is evidence that environment configuration exists; it is not evidence that a secret was exposed.

Behavior change: .env.example and siblings are no longer critical by name. Placeholders, empty values, ${VAR} references, and example credentials produce at most a low/medium informational finding. A real, non-placeholder secret-like value still produces a critical finding — but it names the evidence class and the sensitive key names only, never the value, and rotation advice appears only in that case.

Acceptance criteria completed

1. Template filenames not flagged as exposed secrets — complete (template_present, low).
2. Placeholder values don't trigger critical incidents — complete (empty, ${VAR}/$VAR, <...>, example tokens, and placeholder passwords inside URLs).
3. Actual committed secrets still produce appropriate findings — complete (secret_like_value_detected, critical), including common access-key ID, plural credential, PASSWD, and PWD variants, while path-valued GOOGLE_APPLICATION_CREDENTIALS and CLIENT_ID-style keys remain guarded against false positives.
4. Evidence classification distinguishes template, runtime, or secret-like value — complete (stated in every finding's problem).
5. Recommendations align with evidence (no rotation advice for templates) — complete (rotation only on secret_like_value_detected).
6. Regression tests cover all three scenarios — complete (tests/test_environment_file_review.py, including common credential variants, both negative guards, and the truncation indicator, plus updated tests/test_review_evidence.py).

Testing performed

Run from apps/backend with the project virtualenv (Python 3.13). python -m pytest is what npm run test:backend wraps.

# Focused environment-file and review-evidence regressions
$ python -m pytest tests/test_environment_file_review.py tests/test_review_evidence.py -v
50 passed, 1 warning in 5.30s

# Full backend suite after all review fixes
$ npm run test:backend
502 passed, 4 skipped, 8 warnings in 219.77s

# Targeted frontend integration test after merging latest dev (run from apps/frontend)
$ vitest run src/features/review/components/FindingDetail.test.tsx --config vitest.config.ts --coverage=false
1 test passed (1 file) in 12.74s

The 4 full-suite skips are the PostgreSQL/Redis-gated tests that skip without those services locally (per CONTRIBUTING §10); CI provides them.

Screenshots

Not applicable — no UI change.

Security and data considerations

• Security-sensitive: this logic decides when a critical secret-exposure finding is raised. Please review the placeholder rules and the secret-like-key heuristics closely.
• No secret values are surfaced. Findings, the persisted EnvironmentFileEvidence, logs, tests, and docs carry only evidence classes, file paths, and sensitive key names. Test fixtures use obviously synthetic values, and a regression test asserts the value never appears in the finding.
• No new logging of file contents. Detection reuses the existing bounded _read_text (skips files > 512 KB, ignores unreadable/binary content).
• No migrations, no schema changes, no auth/owner-scoping changes, no untrusted-input handling changes.

Dependencies and blocked work

None.

Scope changes or remaining work

None. The secret-like-value detector is intentionally conservative (key-name heuristics plus embedded-URL credentials); broadening it (e.g. entropy analysis) is out of scope for this false-positive fix.

Contributor checklist

• This PR targets dev
• I claimed the issue and had it assigned or acknowledged before starting substantial work
• The branch was created from an up-to-date dev
• The branch incorporates the latest dev (merged after feat(intelligence): add versioned snapshot query API #126)
• This PR addresses one clearly scoped issue
• Every acceptance criterion I claim as complete is actually complete
• Relevant tests pass
• Documentation is updated for any user-visible change
• No secrets, credentials, local env files, or generated artifacts are included
• No unrelated files were changed
• Closing syntax (Fixes) is used only because the issue is fully resolved
• Dependencies and follow-up work are linked

[parthrohit22]

Thanks for the focused fix and detailed test coverage. I revalidated the exact PR head locally: the focused tests pass, the full backend suite reports 460 passed / 3 skipped, and CI and CodeQL are green.

Before approval, please address these blocking cases:

1. The key-name heuristic still produces critical false positives for ordinary configuration values. For example, PASSWORD_MIN_LENGTH=12, TOKEN_EXPIRY_SECONDS=3600, CLIENT_SECRET_REQUIRED=true, API_KEY_ENABLED=false, and PRIVATE_KEY_PATH=/run/keys/service.pem are all classified as exposed secrets and receive rotation advice. Critical severity needs credible value evidence, not only a security-related key-name match.

2. Dotenv placeholders followed by inline comments are misclassified. Examples include TOKEN=${TOKEN} # access token and API_KEY="${API_KEY}" # required. Please parse quoting and inline comments before placeholder evaluation and add regression coverage.

3. from_record() fully rebuilds cached intelligence whenever an older record has environment files but no new evidence field. Because the rebuilt result is not persisted, every architecture, dependency, review, documentation, analysis, or AI request can rebuild the repository again. Please persist/migrate the compatibility upgrade or use a bounded fallback that does not invalidate the shared cache on every read.

Please add regression tests for these cases, rerun npm run test:backend, and update the acceptance-criteria/checklist claims accordingly. The issue-claim checkbox is also currently unchecked, and Issue #122 contains no claim comment, although it is assigned.

Requesting changes until these points are resolved.

[parthrohit22]

Reviewed against #122 — all six acceptance criteria are implemented and tested, and CI is green.

Particularly good: content wins over filename in both directions (a .env.example with a real secret is still critical), the two-factor rule (sensitive key name + credible value shape) with regression tests for PASSWORD_MIN_LENGTH / CLIENT_SECRET_REQUIRED / PRIVATE_KEY_PATH, the no-value-leakage assertion, and the legacy-cache upgrade guarded by a test that fails if the repo is rebuilt.

One set of small fixes before merge — we're not spinning follow-up issues for changes this small, so please include them here:

1. SECRET_KEY_NAME_PATTERN gaps — a few very common credential key names don't match because of the end-anchor:

   • AWS_ACCESS_KEY_ID (ends in _id) — suggest access_?key(?:_?id)?
   • plural CREDENTIALS (e.g. SERVICE_CREDENTIALS) — suggest credentials?
   • PASSWD / PWD variants (e.g. DB_PASSWD) — suggest adding passwd and pwd

   Something like:

   r"(?:^|_)(?:api_?key|access_?key(?:_?id)?|auth_?token|client_?secret|credentials?|passw(?:or)?d|pwd|private_?key|secret_?key|secret|token)$"
   

[parthrohit22]

Re-reviewed after 5863a31 — the pattern update is exactly right and I independently ran the test files (47/47 pass) and probed the new key families by hand. But my review asked for three things and only one is complete:

1. ✅ Pattern additions — done.

2. ⚠️ Tests — the positive cases are in, but both negative guards I asked for are missing:

   • GOOGLE_APPLICATION_CREDENTIALS=/run/secrets/sa.json → must assert not flagged. This is now the most important test in the set: credentials? matches this ubiquitous key, and only the path-value gate stands between it and a critical false positive. I verified by hand that it behaves correctly today — that behavior needs a test pinning it or the next pattern edit regresses #122 silently.
   • A CLIENT_ID-style key with a credible-shaped value → must assert not flagged.

   Add both to test_security_related_configuration_is_not_credible_secret_evidence (or a sibling).

3. ❌ Truncation indicator — unchanged. The critical finding still joins secret_keys[:10] with no signal when keys were dropped. Append something like " and N more" when len(secret_keys) > 10, with a small test.

These were explicitly requested last round; please address all of them rather than a subset. Once the two guard tests and the truncation fix are in, this is approved — the detection logic itself is verified solid.

[parthrohit22]

Re-reviewed fc3cd00 — all three outstanding items are now complete, and I verified independently: ran both test files in an isolated worktree at this head (50/50 passing) and confirmed the commit touches only the two expected files.

• The GOOGLE_APPLICATION_CREDENTIALS guard test pins exactly the interaction that mattered, and CLIENT_ID=client-Prod-12345678 was a well-chosen value — credible-shaped, so it proves the key-name gate does the rejecting rather than the value gate.
• Truncation now reports "and N more" only when keys were omitted, with the ≤10 text unchanged, and the test confirms the full key list stays intact in the evidence.

Approving. This closes out #122 properly — evidence-first classification, no value leakage, and every behavior pinned by a test. Nice work across the three rounds.