fix(continual-learning): harden wasRealCollision verifier (Codex review)
Addresses the P1 brittleness in the Step 3 verifier so learned_from edges (graph/live.ts:413) can't be silently zeroed on stage. - Capture overlap AT detection time as Collision.gitOverlap (podman.ts), while engineer_states are still fresh, instead of re-deriving from possibly-stale state when the user clicks. deriveWasRealCollision() now prefers this stored evidence and only falls back to a live re-derivation for older collisions. - Canonicalize engineer names (trim + lowercase) on both the capture and the fallback path, so "Karti" vs "karti" no longer misses the git state. - conflictKey now reuses the shared comparableBasename() helper (dedupe). shared/src/collision.ts gains optional `gitOverlap?: boolean` (additive). shared rebuilt; backend+frontend typecheck + eslint pass. PLAN.md rung 3 updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -9,11 +9,42 @@ import {
|
|||||||
recordIntervention,
|
recordIntervention,
|
||||||
updateInterventionStatus,
|
updateInterventionStatus,
|
||||||
} from '../memory/store.js';
|
} from '../memory/store.js';
|
||||||
import { getGitStates } from '../memory/db.js';
|
import { getGitStates, type GitState } from '../memory/db.js';
|
||||||
import { recallSimilar } from '../memory/vectors.js';
|
import { recallSimilar } from '../memory/vectors.js';
|
||||||
import { shouldIntervene, preferredAction } from '../memory/policy.js';
|
import { shouldIntervene, preferredAction } from '../memory/policy.js';
|
||||||
import { publishHermesIntervention } from '../action/hermes.js';
|
import { publishHermesIntervention } from '../action/hermes.js';
|
||||||
|
|
||||||
|
/** Strip a git-status prefix ("M ", "?? ") and reduce a path to its lowercased
|
||||||
|
* basename — matches comparableFile() in memory/store.ts so keys line up. */
|
||||||
|
function comparableBasename(raw?: string): string {
|
||||||
|
return (
|
||||||
|
(raw ?? '')
|
||||||
|
.trim()
|
||||||
|
.replace(/^(\?\?|[MADRCU!]{1,2})\s+/, '')
|
||||||
|
.split(/[\\/]/)
|
||||||
|
.pop()
|
||||||
|
?.toLowerCase() ?? ''
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Canonicalize an engineer name for case/whitespace-insensitive matching, so
|
||||||
|
* "Karti" and "karti" resolve to the same engineer's git state. */
|
||||||
|
function canonicalName(raw?: string): string {
|
||||||
|
return (raw ?? '').trim().toLowerCase();
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Git ground truth: do ALL involved engineers currently have the collided file
|
||||||
|
* in their changedFiles? Computed at detection time while git state is fresh. */
|
||||||
|
function engineersOverlapOnFile(collision: Collision, gitStates: Map<string, GitState>): boolean {
|
||||||
|
const target = comparableBasename(collision.file);
|
||||||
|
if (!target || collision.engineers.length < 2) return false;
|
||||||
|
const byCanon = new Map<string, string[]>();
|
||||||
|
for (const [name, st] of gitStates) byCanon.set(canonicalName(name), st.changedFiles);
|
||||||
|
return collision.engineers.every((e) =>
|
||||||
|
(byCanon.get(canonicalName(e)) ?? []).some((f) => comparableBasename(f) === target),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
export class PodMan {
|
export class PodMan {
|
||||||
private contexts = new Map<string, EngineerContext>();
|
private contexts = new Map<string, EngineerContext>();
|
||||||
/**
|
/**
|
||||||
@@ -75,6 +106,12 @@ export class PodMan {
|
|||||||
if (!current.has(key)) this.activeConflicts.delete(key);
|
if (!current.has(key)) this.activeConflicts.delete(key);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Capture git ground-truth overlap now, while engineer_states are fresh, so
|
||||||
|
// the outcome-time verifier never depends on a stale sidecar or a late click.
|
||||||
|
for (const collision of collisions) {
|
||||||
|
collision.gitOverlap = engineersOverlapOnFile(collision, gitStates);
|
||||||
|
}
|
||||||
|
|
||||||
for (const collision of collisions) await this.handle(collision);
|
for (const collision of collisions) await this.handle(collision);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -85,14 +122,7 @@ export class PodMan {
|
|||||||
* basename.
|
* basename.
|
||||||
*/
|
*/
|
||||||
private conflictKey(collision: Collision): string {
|
private conflictKey(collision: Collision): string {
|
||||||
return (
|
return comparableBasename(collision.file);
|
||||||
(collision.file ?? '')
|
|
||||||
.trim()
|
|
||||||
.replace(/^(\?\?|[MADRCU!]{1,2})\s+/, '')
|
|
||||||
.split(/[\\/]/)
|
|
||||||
.pop()
|
|
||||||
?.toLowerCase() ?? ''
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
private async handle(collision: Collision): Promise<void> {
|
private async handle(collision: Collision): Promise<void> {
|
||||||
|
|||||||
@@ -96,15 +96,22 @@ export async function deriveWasRealCollision(outcome: InterventionOutcome): Prom
|
|||||||
try {
|
try {
|
||||||
const c = await collections();
|
const c = await collections();
|
||||||
const collision = await c.collisions.findOne({ id: outcome.collisionId });
|
const collision = await c.collisions.findOne({ id: outcome.collisionId });
|
||||||
if (!collision || !Array.isArray(collision.engineers) || collision.engineers.length < 2) {
|
if (!collision) return false;
|
||||||
return false;
|
// Prefer the overlap evidence captured at detection time (fresh git state):
|
||||||
}
|
// immune to late clicks, stale sidecars, and the engineer_states TTL.
|
||||||
|
if (typeof collision.gitOverlap === 'boolean') return collision.gitOverlap;
|
||||||
|
// Fallback for collisions detected before gitOverlap was captured: re-derive
|
||||||
|
// from latest git state, matching engineers on case/whitespace-canonical names.
|
||||||
|
if (!Array.isArray(collision.engineers) || collision.engineers.length < 2) return false;
|
||||||
const target = comparableFile(collision.file);
|
const target = comparableFile(collision.file);
|
||||||
if (!target) return false;
|
if (!target) return false;
|
||||||
const gitStates = await getGitStates(outcome.podId);
|
const byCanon = new Map<string, string[]>();
|
||||||
const touchesTarget = (name: string): boolean =>
|
for (const [name, st] of await getGitStates(outcome.podId)) {
|
||||||
(gitStates.get(name)?.changedFiles ?? []).some((f) => comparableFile(f) === target);
|
byCanon.set(name.trim().toLowerCase(), st.changedFiles);
|
||||||
return collision.engineers.every(touchesTarget);
|
}
|
||||||
|
return collision.engineers.every((e) =>
|
||||||
|
(byCanon.get(e.trim().toLowerCase()) ?? []).some((f) => comparableFile(f) === target),
|
||||||
|
);
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
console.error(`[memory] wasRealCollision verifier failed: ${(err as Error).message}`);
|
console.error(`[memory] wasRealCollision verifier failed: ${(err as Error).message}`);
|
||||||
return false;
|
return false;
|
||||||
|
|||||||
+12
-7
@@ -510,14 +510,19 @@ no schema change. Owner: RSI track. Independent of the MongoDB-cleanup handoff.
|
|||||||
kind), `plan.md:66` (second similar event behaves differently).
|
kind), `plan.md:66` (second similar event behaves differently).
|
||||||
|
|
||||||
3. **Step 3 - derive `wasRealCollision` from git overlap (backend-authoritative)** ✅
|
3. **Step 3 - derive `wasRealCollision` from git overlap (backend-authoritative)** ✅
|
||||||
- `backend/src/memory/store.ts` `recordOutcome` now overrides the
|
- Overlap is captured AT detection time as `Collision.gitOverlap`
|
||||||
client-supplied `wasRealCollision` with `deriveWasRealCollision()`: a
|
(`backend/src/agent/podman.ts`), while `engineer_states` are still fresh —
|
||||||
collision is real only if BOTH named engineers currently have the collided
|
true only if ALL involved engineers have the collided file in their git
|
||||||
file in their git `changedFiles` (`getGitStates`, 120s freshness TTL).
|
`changedFiles`, matched on case/whitespace-canonical names.
|
||||||
Conservative `false` when orphaned/stale. `frontend/.../useInterventions.ts`
|
- `backend/src/memory/store.ts` `recordOutcome` overrides the client value
|
||||||
stops sending a hardcoded `true` (now a backend-overridden placeholder).
|
with `deriveWasRealCollision()`, which prefers the stored `gitOverlap`
|
||||||
- Restores the (accepted × wasReal) 2×2 the spec assumes.
|
(immune to late clicks / stale sidecars / the 120s TTL) and only falls back
|
||||||
|
to a live canonical-name re-derivation for pre-existing collisions.
|
||||||
|
`frontend/.../useInterventions.ts` stops sending hardcoded `true`.
|
||||||
|
- Restores the (accepted × wasReal) 2×2 the spec assumes; keeps `learned_from`
|
||||||
|
edges (`graph/live.ts:413`) from being silently zeroed on stage.
|
||||||
- Spec: `docs/continual-learning/spec.md:98-108`, `policy.md:35-42`.
|
- Spec: `docs/continual-learning/spec.md:98-108`, `policy.md:35-42`.
|
||||||
|
- Hardened per Codex review (name canonicalization + detection-time capture).
|
||||||
|
|
||||||
Follow-ups (separate rungs, not in this change): Step 4-5 `strategy_versions` +
|
Follow-ups (separate rungs, not in this change): Step 4-5 `strategy_versions` +
|
||||||
Gemini-proposed `LearningProposal` slice; Step 6 durable `owns` write; seed a
|
Gemini-proposed `LearningProposal` slice; Step 6 durable `owns` write; seed a
|
||||||
|
|||||||
@@ -16,6 +16,14 @@ export interface Collision {
|
|||||||
severity: CollisionSeverity;
|
severity: CollisionSeverity;
|
||||||
/** Snapshot of relevant GitHub state at detection time. */
|
/** Snapshot of relevant GitHub state at detection time. */
|
||||||
githubState?: GithubStateSnapshot;
|
githubState?: GithubStateSnapshot;
|
||||||
|
/**
|
||||||
|
* Git ground-truth overlap captured AT detection time, while engineer_states
|
||||||
|
* are still fresh: true when every involved engineer had `file` in their git
|
||||||
|
* changedFiles. Read as the authoritative wasRealCollision evidence at outcome
|
||||||
|
* time, so a late click, a stale sidecar, or the engineer_states freshness TTL
|
||||||
|
* cannot retroactively zero it out.
|
||||||
|
*/
|
||||||
|
gitOverlap?: boolean;
|
||||||
detectedAt: string;
|
detectedAt: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user