[codex] add evidence-backed failure mining - #5
Conversation
…n-lifecycle-verifiers # Conflicts: # src/ofw/__init__.py
…mission-snapshots # Conflicts: # src/ofw/__init__.py
divo12
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ofw/mine.py">
<violation number="1" location="src/ofw/mine.py:606">
P2: When the nominated trace has at least 100 matching segments, prior search filters those 100 current-trace hits and omits later prior traces. Exclude the current trace in the query or page until the requested prior hits are collected.</violation>
<violation number="2" location="src/ofw/mine.py:882">
P1: A judge can relabel a verifier's legitimate environment evidence as a different completion status and pass `_judge_result_matches`. Retain each verification result and require every completion check's status and observed state to match that verification before accepting the verdict.</violation>
</file>
<file name="src/ofw/runtime.py">
<violation number="1" location="src/ofw/runtime.py:396">
P3: Removing runtime_configuration leaves the fingerprint machinery dead code: E2BSandbox.fingerprint, CommandLoop.fingerprint, CommandVerifier.fingerprint, and _command_fingerprint now have no callers anywhere in src/ or tests/. Delete the now-unused fingerprint methods, _command_fingerprint, and their helpers (or keep them only if the new identity scheme still needs them).</violation>
</file>
<file name="tests/test_mine.py">
<violation number="1" location="tests/test_mine.py:613">
P3: This test's name and the `FailureMiningResult` guard claim that a confirmed failure requires both trajectory and environment evidence, but the fixture only removes `environment_evidence` (trajectory evidence stays populated), so only the missing-environment branch is exercised. The missing-trajectory branch of the `CONFIRMED_FAILURE` validation is actually unreachable from construction: `BehaviorObservation` requires non-empty trajectory evidence, and `_validate_behavior` runs before the verdict branch and rejects behavior evidence that is not a subset of `trajectory_evidence` (raising "failure behavior observation is outside the grounded context"). Rename the test to reflect that it only checks the environment requirement (`test_confirmed_failure_requires_environment_evidence`), or, if coverage of the trajectory requirement matters, test that clause through other means rather than implying this fixture covers it.</violation>
</file>
<file name="src/ofw/repository.py">
<violation number="1" location="src/ofw/repository.py:38">
P3: process_repository constructs a HarnessRevisionContent only to copy its fields into the revision, and HarnessRevisionContent.canonical_json/_render_content are never called anywhere in the codebase (only defined in contracts.py). After the component/runtime removal in this PR these render helpers are dead code. Either drop the throwaway content object and build HarnessRevision directly, or remove canonical_json/_render_content to avoid maintaining an unused serialization path that can drift from HarnessRevision.to_json.</violation>
<violation number="2" location="src/ofw/repository.py:44">
P2: The revision id encodes only the git commit/dirty digest, so two process_repository calls with different observability (traces=None vs traces=project) return the same id but write different manifests to the same path (root/.ofw/revisions/<id>/manifest.json). The second _write_manifest silently overwrites the first, so the persisted 'immutable' manifest depends on call order and last-write-wins. tests/test_repository.py and tests/test_langfuse_contracts.py both assert connected.id == baseline.id, confirming the collision. Keep observability out of code identity if intended, but don't key the on-disk manifest only by id when the persisted content varies; otherwise a later call drops the earlier manifest's observability state.</violation>
<violation number="3" location="src/ofw/repository.py:105">
P2: When Git EOL or clean-filter normalization changes raw tracked worktree bytes, this `git diff` can report no change although the runtime evaluates those bytes. Hash raw worktree contents for tracked files, while separately representing deletions and modes, instead of using normalized diff output.</violation>
<violation number="4" location="src/ofw/repository.py:112">
P3: When reading untracked files, path.read_bytes() can raise OSError (permission denied, file removed between ls-files and read, non-regular file) and os.readlink/path checks are not wrapped. _dirty_payload is called from _snapshot_repository with no surrounding try/except, so these raw OSErrors escape process_repository instead of being converted to HarnessValidationError like every other git/IO failure in this module. Wrap the untracked-file read so a single unreadable file fails with a typed HarnessValidationError rather than an unexpected OSError.</violation>
<violation number="5" location="src/ofw/repository.py:113">
P1: When an untracked file contains the record marker, distinct untracked-file sets receive the same dirty revision ID. Length-prefix each path and content, or otherwise use unambiguous canonical records.</violation>
<violation number="6" location="src/ofw/repository.py:116">
P2: When an untracked executable changes permissions, `_dirty_payload` keeps the same revision ID even though the runtime workspace preserves those permissions. Include the untracked entry's mode in the canonical payload.</violation>
</file>
<file name="tests/test_runtime.py">
<violation number="1" location="tests/test_runtime.py:220">
P3: The digest assertion is tautological: `CanaryReport.digest` is always constructed as `sha256:...`, so `startswith("sha256:")` passes for any report and does not verify that evidence is actually frozen or recorded. Assert on something meaningful instead, e.g. run the canary twice and compare `report.digest` equality, or verify recorded evidence output as the replaced `test_process_runs_e2b_command_canary_and_records_frozen_evidence` did.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| and all(item.kind is EvidenceKind.ENVIRONMENT for item in completion_evidence) | ||
| and all(item in issued_evidence for item in result.trajectory_evidence) | ||
| and all(item in issued_evidence for item in result.environment_evidence) | ||
| and all(item in issued_evidence for item in completion_evidence) |
There was a problem hiding this comment.
P1: A judge can relabel a verifier's legitimate environment evidence as a different completion status and pass _judge_result_matches. Retain each verification result and require every completion check's status and observed state to match that verification before accepting the verdict.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/mine.py, line 882:
<comment>A judge can relabel a verifier's legitimate environment evidence as a different completion status and pass `_judge_result_matches`. Retain each verification result and require every completion check's status and observed state to match that verification before accepting the verdict.</comment>
<file context>
@@ -0,0 +1,998 @@
+ and all(item.kind is EvidenceKind.ENVIRONMENT for item in completion_evidence)
+ and all(item in issued_evidence for item in result.trajectory_evidence)
+ and all(item in issued_evidence for item in result.environment_evidence)
+ and all(item in issued_evidence for item in completion_evidence)
+ and all(item in issued_evidence for item in behavior_evidence)
+ )
</file context>
| continue | ||
| path = root / relative | ||
| content = os.fsencode(os.readlink(path)) if path.is_symlink() else path.read_bytes() | ||
| payload.extend(b"\0untracked\0") |
There was a problem hiding this comment.
P1: When an untracked file contains the record marker, distinct untracked-file sets receive the same dirty revision ID. Length-prefix each path and content, or otherwise use unambiguous canonical records.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/repository.py, line 113:
<comment>When an untracked file contains the record marker, distinct untracked-file sets receive the same dirty revision ID. Length-prefix each path and content, or otherwise use unambiguous canonical records.</comment>
<file context>
@@ -0,0 +1,181 @@
+ continue
+ path = root / relative
+ content = os.fsencode(os.readlink(path)) if path.is_symlink() else path.read_bytes()
+ payload.extend(b"\0untracked\0")
+ payload.extend(encoded_path)
+ payload.extend(b"\0")
</file context>
| ) -> TrajectorySearchResult: | ||
| store = CollectionStore(self.collection.store_path) | ||
| try: | ||
| limit = 100 if prior_only else request.limit |
There was a problem hiding this comment.
P2: When the nominated trace has at least 100 matching segments, prior search filters those 100 current-trace hits and omits later prior traces. Exclude the current trace in the query or page until the requested prior hits are collected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/mine.py, line 606:
<comment>When the nominated trace has at least 100 matching segments, prior search filters those 100 current-trace hits and omits later prior traces. Exclude the current trace in the query or page until the requested prior hits are collected.</comment>
<file context>
@@ -0,0 +1,998 @@
+ ) -> TrajectorySearchResult:
+ store = CollectionStore(self.collection.store_path)
+ try:
+ limit = 100 if prior_only else request.limit
+ hits = self._search_phrase(store, request, trace_id, request.text, limit)
+ if not hits:
</file context>
|
|
||
|
|
||
| def _dirty_payload(root: Path) -> bytes: | ||
| payload = bytearray(_run_git(root, "diff", "--binary", "--no-ext-diff", "HEAD", "--")) |
There was a problem hiding this comment.
P2: When Git EOL or clean-filter normalization changes raw tracked worktree bytes, this git diff can report no change although the runtime evaluates those bytes. Hash raw worktree contents for tracked files, while separately representing deletions and modes, instead of using normalized diff output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/repository.py, line 105:
<comment>When Git EOL or clean-filter normalization changes raw tracked worktree bytes, this `git diff` can report no change although the runtime evaluates those bytes. Hash raw worktree contents for tracked files, while separately representing deletions and modes, instead of using normalized diff output.</comment>
<file context>
@@ -0,0 +1,181 @@
+
+
+def _dirty_payload(root: Path) -> bytes:
+ payload = bytearray(_run_git(root, "diff", "--binary", "--no-ext-diff", "HEAD", "--"))
+ untracked = _run_git(root, "ls-files", "--others", "--exclude-standard", "-z")
+ for encoded_path in sorted(item for item in untracked.split(b"\0") if item):
</file context>
| payload.extend(b"\0untracked\0") | ||
| payload.extend(encoded_path) | ||
| payload.extend(b"\0") | ||
| payload.extend(content) |
There was a problem hiding this comment.
P2: When an untracked executable changes permissions, _dirty_payload keeps the same revision ID even though the runtime workspace preserves those permissions. Include the untracked entry's mode in the canonical payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/repository.py, line 116:
<comment>When an untracked executable changes permissions, `_dirty_payload` keeps the same revision ID even though the runtime workspace preserves those permissions. Include the untracked entry's mode in the canonical payload.</comment>
<file context>
@@ -0,0 +1,181 @@
+ payload.extend(b"\0untracked\0")
+ payload.extend(encoded_path)
+ payload.extend(b"\0")
+ payload.extend(content)
+ return bytes(payload)
+
</file context>
| _CANARY_ADAPTER: TypeAdapter[CanaryReport] = TypeAdapter(CanaryReport) | ||
|
|
||
|
|
||
| def runtime_configuration( |
There was a problem hiding this comment.
P3: Removing runtime_configuration leaves the fingerprint machinery dead code: E2BSandbox.fingerprint, CommandLoop.fingerprint, CommandVerifier.fingerprint, and _command_fingerprint now have no callers anywhere in src/ or tests/. Delete the now-unused fingerprint methods, _command_fingerprint, and their helpers (or keep them only if the new identity scheme still needs them).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/runtime.py, line 396:
<comment>Removing runtime_configuration leaves the fingerprint machinery dead code: E2BSandbox.fingerprint, CommandLoop.fingerprint, CommandVerifier.fingerprint, and _command_fingerprint now have no callers anywhere in src/ or tests/. Delete the now-unused fingerprint methods, _command_fingerprint, and their helpers (or keep them only if the new identity scheme still needs them).</comment>
<file context>
@@ -393,19 +393,6 @@ def to_json(self) -> str:
- )
-
-
def run_canary(
revision: HarnessRevision,
case: CanaryCase,
</file context>
| assert result.invalid_reason is reason | ||
|
|
||
|
|
||
| def test_confirmed_failure_requires_trajectory_and_environment_evidence() -> None: |
There was a problem hiding this comment.
P3: This test's name and the FailureMiningResult guard claim that a confirmed failure requires both trajectory and environment evidence, but the fixture only removes environment_evidence (trajectory evidence stays populated), so only the missing-environment branch is exercised. The missing-trajectory branch of the CONFIRMED_FAILURE validation is actually unreachable from construction: BehaviorObservation requires non-empty trajectory evidence, and _validate_behavior runs before the verdict branch and rejects behavior evidence that is not a subset of trajectory_evidence (raising "failure behavior observation is outside the grounded context"). Rename the test to reflect that it only checks the environment requirement (test_confirmed_failure_requires_environment_evidence), or, if coverage of the trajectory requirement matters, test that clause through other means rather than implying this fixture covers it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_mine.py, line 613:
<comment>This test's name and the `FailureMiningResult` guard claim that a confirmed failure requires both trajectory and environment evidence, but the fixture only removes `environment_evidence` (trajectory evidence stays populated), so only the missing-environment branch is exercised. The missing-trajectory branch of the `CONFIRMED_FAILURE` validation is actually unreachable from construction: `BehaviorObservation` requires non-empty trajectory evidence, and `_validate_behavior` runs before the verdict branch and rejects behavior evidence that is not a subset of `trajectory_evidence` (raising "failure behavior observation is outside the grounded context"). Rename the test to reflect that it only checks the environment requirement (`test_confirmed_failure_requires_environment_evidence`), or, if coverage of the trajectory requirement matters, test that clause through other means rather than implying this fixture covers it.</comment>
<file context>
@@ -0,0 +1,870 @@
+ assert result.invalid_reason is reason
+
+
+def test_confirmed_failure_requires_trajectory_and_environment_evidence() -> None:
+ with pytest.raises(ValueError, match="confirmed failure requires"):
+ FailureMiningResult(
</file context>
| def test_confirmed_failure_requires_trajectory_and_environment_evidence() -> None: | |
| def test_confirmed_failure_requires_environment_evidence() -> None: |
|
|
||
| assert raised.value.code is HarnessErrorCode.DUPLICATE_VERIFIER | ||
| assert report.passed | ||
| assert str(report.digest).startswith("sha256:") |
There was a problem hiding this comment.
P3: The digest assertion is tautological: CanaryReport.digest is always constructed as sha256:..., so startswith("sha256:") passes for any report and does not verify that evidence is actually frozen or recorded. Assert on something meaningful instead, e.g. run the canary twice and compare report.digest equality, or verify recorded evidence output as the replaced test_process_runs_e2b_command_canary_and_records_frozen_evidence did.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_runtime.py, line 220:
<comment>The digest assertion is tautological: `CanaryReport.digest` is always constructed as `sha256:...`, so `startswith("sha256:")` passes for any report and does not verify that evidence is actually frozen or recorded. Assert on something meaningful instead, e.g. run the canary twice and compare `report.digest` equality, or verify recorded evidence output as the replaced `test_process_runs_e2b_command_canary_and_records_frozen_evidence` did.</comment>
<file context>
@@ -208,77 +204,36 @@ def _command_loop() -> CommandLoop:
- assert raised.value.code is HarnessErrorCode.DUPLICATE_VERIFIER
+ assert report.passed
+ assert str(report.digest).startswith("sha256:")
</file context>
| if _ignored_internal_path(relative): | ||
| continue | ||
| path = root / relative | ||
| content = os.fsencode(os.readlink(path)) if path.is_symlink() else path.read_bytes() |
There was a problem hiding this comment.
P3: When reading untracked files, path.read_bytes() can raise OSError (permission denied, file removed between ls-files and read, non-regular file) and os.readlink/path checks are not wrapped. _dirty_payload is called from _snapshot_repository with no surrounding try/except, so these raw OSErrors escape process_repository instead of being converted to HarnessValidationError like every other git/IO failure in this module. Wrap the untracked-file read so a single unreadable file fails with a typed HarnessValidationError rather than an unexpected OSError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/repository.py, line 112:
<comment>When reading untracked files, path.read_bytes() can raise OSError (permission denied, file removed between ls-files and read, non-regular file) and os.readlink/path checks are not wrapped. _dirty_payload is called from _snapshot_repository with no surrounding try/except, so these raw OSErrors escape process_repository instead of being converted to HarnessValidationError like every other git/IO failure in this module. Wrap the untracked-file read so a single unreadable file fails with a typed HarnessValidationError rather than an unexpected OSError.</comment>
<file context>
@@ -0,0 +1,181 @@
+ if _ignored_internal_path(relative):
+ continue
+ path = root / relative
+ content = os.fsencode(os.readlink(path)) if path.is_symlink() else path.read_bytes()
+ payload.extend(b"\0untracked\0")
+ payload.extend(encoded_path)
</file context>
| if _NAME_PATTERN.fullmatch(name) is None: | ||
| raise HarnessValidationError(HarnessErrorCode.INVALID_NAME, name) | ||
| selected_root = _resolve_root(root) | ||
| content = HarnessRevisionContent( |
There was a problem hiding this comment.
P3: process_repository constructs a HarnessRevisionContent only to copy its fields into the revision, and HarnessRevisionContent.canonical_json/_render_content are never called anywhere in the codebase (only defined in contracts.py). After the component/runtime removal in this PR these render helpers are dead code. Either drop the throwaway content object and build HarnessRevision directly, or remove canonical_json/_render_content to avoid maintaining an unused serialization path that can drift from HarnessRevision.to_json.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/ofw/repository.py, line 38:
<comment>process_repository constructs a HarnessRevisionContent only to copy its fields into the revision, and HarnessRevisionContent.canonical_json/_render_content are never called anywhere in the codebase (only defined in contracts.py). After the component/runtime removal in this PR these render helpers are dead code. Either drop the throwaway content object and build HarnessRevision directly, or remove canonical_json/_render_content to avoid maintaining an unused serialization path that can drift from HarnessRevision.to_json.</comment>
<file context>
@@ -0,0 +1,181 @@
+ if _NAME_PATTERN.fullmatch(name) is None:
+ raise HarnessValidationError(HarnessErrorCode.INVALID_NAME, name)
+ selected_root = _resolve_root(root)
+ content = HarnessRevisionContent(
+ schema_version=HarnessSchemaVersion.V1,
+ harness_name=name,
</file context>
1 participant
What this PR does
connect_*mappingExplicitly out of scope
Verification