feat: Phases 4–6 intelligence, retention, and adaptation - #5
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 35799352 | Triggered | JSON Web Token | 26038f0 | crates/tif-core/src/audit.rs | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| #[test] | ||
| fn redacts_jwt_and_connection_string() { | ||
| let jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.signaturepart"; |
There was a problem hiding this comment.
🔥 The Roast: Your "JWT" here is the canonical example token from jwt.io / RFC 7519 — and GitGuardian agreed so loudly it filed Incident #35799352 against your own PR. Nothing says "production-grade security tooling" like triggering a secret scanner with your test fixture. A reviewer running GitGuardian in their CI now has to whitelist this PR or pretend eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9 isn't shaped exactly like the kind of token you claim to redact.
🩹 The Fix:
| let jwt = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.signaturepart"; | |
| let jwt = "eyJ-TEST-FIXTURE-NOT-A-REAL-TOKEN.sig.placeholder_signature_with_enough_length_for_match"; |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| continue; | ||
| } | ||
| let new_text = fs::read_to_string(repo_root.join(p)).unwrap_or_default(); | ||
| let old_text = git_output(repo_root, &["show", &format!("HEAD:{p}")]).unwrap_or_default(); |
There was a problem hiding this comment.
🔥 The Roast: git show HEAD:<path> on an untracked manifest returns empty, so dependency_delta_from_texts(name, "", &new_text) cheerfully reports every single dep in that brand-new Cargo.toml as "added". First commit of any fresh repo suddenly looks like a 12-package dependency explosion — the simplicity score goes feral, OutOfControl fires, and Firebreak wakes up for a greenfield project that literally has no prior baseline to compare to. Bonus points: same wrong result fires for renamed manifests and for manifests present in HEAD but absent from a fresh clone's working tree.
🩹 The Fix: Treat an empty old_text (or a git show failure) as "no prior baseline" rather than "zero deps":
| let old_text = git_output(repo_root, &["show", &format!("HEAD:{p}")]).unwrap_or_default(); | |
| let old_text = match git_output(repo_root, &["show", &format!("HEAD:{p}")]) { | |
| Ok(t) if !t.is_empty() => t, | |
| _ => continue, | |
| }; |
📏 Severity: critical
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if let Ok(age) = modified.elapsed() { | ||
| if age.as_secs() > 2 * 60 * 60 { | ||
| let _ = fs::remove_file(&path); | ||
| return Self::acquire(state_dir); |
There was a problem hiding this comment.
🔥 The Roast: "Exclusive lock" that, on a stale file, does remove_file and then recursively re-acquires. Two processes spot the same 2-hour-old lock, both call remove_file, both call acquire again, and both walk away thinking they hold the only lock in town. The docstring above promises "Fails if another process holds the lockfile" — it doesn't, and a long-running apply (large repo, slow disk, container with throttled IO) can be silently stomped exactly when it's most fragile.
🩹 The Fix: Use flock(2) via fs2/nix, or at minimum record the original holder's PID inside the lockfile and refuse to take over unless that PID is verifiably dead (kill(pid, 0) == ESRCH). The recursive call also risks unbounded stack on a stubborn stale-lock loop — convert to a loop, max one retry.
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for w in weakened { | ||
| out.push(TestChange { | ||
| path: path.to_string(), | ||
| reason: format!("weakened assertion pattern: {w}"), |
There was a problem hiding this comment.
🔥 The Roast: format!("weakened assertion pattern: {w}") stuffs the entire raw source line of a test file into the Damage Assessment. With audit.tier = "full" that line — including any embedded secret, API URL, customer email, or PHI in a fixture — gets persisted into .this-is-fine/audit.db and on-disk artifacts verbatim. The redaction pipeline (redact_secrets) never sees it. You built a secret-redaction subsystem and then immediately routed around it.
🩹 The Fix: Don't echo source. Record the matched pattern token only (e.g. the category like assert!(true) / #[ignore] / t.skip) plus path and a truncated snippet capped at ~40 chars:
| reason: format!("weakened assertion pattern: {w}"), | |
| reason: format!("weakened assertion pattern: {}", category_for(&w)), |
📏 Severity: warning
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: Request changes | Recommendation: Two critical correctness/security findings must be fixed before merge. Overview
Issue Details (click to expand)
🏆 Best part: The pressure-engine rewrite. Two versioned variants per family, a real promotion/demotion gate with floor-fail-rate blocking, preferred-template selection, and the self-apply allowlist actually rejects Five-Alarm at the type level. That's how you ship adaptation without making the rest of the system nervous. 💀 Worst part: 📊 Overall: Like a smoke detector that goes off every time you boil water — the sensing hardware is solid, but somebody calibrated the threshold with a sledgehammer. Fix the two criticals, tighten the lockfile semantics, and this lands clean. Correctness / Safety Findings
Ponytail Review
Ponytail net: -13 lines. Suggested Minimal Patch
Final Merge GuidanceDo not merge until the two critical findings (JWT fixture, dependency-delta false positives) are fixed. The two warnings are strongly recommended but not blocking. Fix these issues in Kilo Cloud Files Reviewed (18 files)
Reviewed by minimax-m3 · Input: 122.8K · Output: 12.5K · Cached: 2.4M |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26038f0dec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| apply_generated_code_heuristics(&mut metrics, Some(repo_root)); | ||
| metrics.changed_paths = paths; |
There was a problem hiding this comment.
When metrics_from_git handles the normal --from-git / working-tree path, apply_generated_code_heuristics reads metrics.changed_paths, but this vector is still empty here and is only assigned on the next line. In that scenario generated files and lockfiles never receive the new generated_code_lines penalty for git-based assessments, unlike tree/unified-diff assessments. Move the changed_paths assignment before invoking the heuristic.
Useful? React with 👍 / 👎.
| // Explicitly never apply floor / sensitive / verify knobs. | ||
| store.save_adaptation_stats(eng.stats())?; |
There was a problem hiding this comment.
When a user runs tif adaptation recommend --apply, this path persists fire-level bias, threshold scale, and pressure template choices in the audit DB, but the policy paths I inspected never read those saved knobs: cmd_run_begin still compiles with adaptive_limit_scale: None, and PolicyCompiler::compile still builds pressure selection only from config without a saved fire-level bias or preferred templates. In that scenario the command reports self-applied successfully while the next tif run begin behaves exactly as before, making the new self-apply feature a no-op.
Useful? React with 👍 / 👎.
Summary
Implements production Phases 4–6 for This Is Fine.
Phase 4 — Intelligence depth
confident: falseon weak evidencescoring_version, test-change notes[simplicity.exceptions]audit notesPhase 5 — Audit / privacy / retention
tif audit --gcwires audit + isolation GC with rollback retentionPhase 6 — Adaptation
tif adaptation status|recommend|resetValidation
cargo fmtcargo clippy --all-features -- -D warningscargo test --workspace --all-features→ 157 passedDocs