Skip to content

fix(learn): fail-closed episodic mirror before candidate publish - #71

Open
diazMelgarejo wants to merge 7 commits into
codejunkie99:masterfrom
diazMelgarejo:atomic-01b-episodic-mirror-fail-closed
Open

diazMelgarejo wants to merge 7 commits into
codejunkie99:masterfrom
diazMelgarejo:atomic-01b-episodic-mirror-fail-closed

Conversation

@diazMelgarejo

@diazMelgarejo diazMelgarejo commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Manual stage() previously wrote evidence_ids: [now] then fail-open-mirrored into AGENT_LEARNINGS.jsonl, so a mirror write error left dangling evidence.
  • Publish the candidate via temp file + os.replace only after append_jsonl mirror succeeds (fail-closed).
  • Update stdlib unittest accordingly (3/3 pass).

Found and hardened downstream in Perpetua-Tools; contributed back without PT-local path_hygiene.

Test plan

  • cd .agent/tools && python3 -m unittest test_learn_episodic_mirror -v (3/3)
  • Maintainer review of fail-closed vs prior fail-open posture

Made with Cursor

Note

Make learn candidate staging fail-closed on episodic mirror errors

  • Reworks learn.stage so the episodic mirror row is written first and durably, and the candidate JSON is published only after that evidence exists. The mirror append is now append-once under an exclusive lock, so retries reuse one canonical timestamp instead of adding duplicate rows.
  • Stages the candidate through a temporary file with flush, fsync, and directory sync, and replaces it into place at publish time. Before each new stage run, leftover temporary candidates are recovered: valid ones are published, stale or corrupt ones are discarded, and identical twins are collapsed to one candidate.
  • Adds JSONL helpers in episodic_io.py for append-once, locked timestamp lookup, and fsync-before-unlock. Hooks in on_failure.py and post_execution.py now catch OSError from the episodic append and warn on stderr instead of failing.
  • Extensive regression coverage added in test_learn_episodic_mirror.py and test_episodic_hooks.py.
  • Behavioral Change: staging errors now cause the learn command to exit with a failure status, and evidence validation uses exact parsed JSONL timestamp fields instead of raw substring matching, so substring-only matches no longer count as evidence.

Macroscope summarized 4b56ff0.

RetriggerConfidence Score: 3/5

The PR is not yet safe to merge because concurrent recovery can discard candidate content and a malformed episodic entry can stop manual staging.

Reviews (4) · Last reviewed commit: "Merge pull request #3 from diazMelgarejo..."

Manual stage() previously wrote evidence_ids then fail-open-mirrored
into AGENT_LEARNINGS.jsonl, so a mirror write error left dangling
evidence. Publish candidate via temp+os.replace only after append_jsonl
mirror succeeds. No path_hygiene (PT-local stays out of upstream).

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread .agent/tools/learn.py Outdated
Co-authored-by: cyre <Lawrence@bettermind.ph>
Match evidence on a parsed timestamp. Keep the temp when the mirror
line is already readable, and do not treat a read error as corrupt
JSON. A directory fsync after rename cannot force another append.

Co-authored-by: cyre <Lawrence@bettermind.ph>
Comment thread .agent/tools/learn.py
Comment thread .agent/tools/learn.py Outdated
Comment thread .agent/harness/hooks/_episodic_io.py
Reuse the earliest manual-stage row for a candidate id under the
existing episodic flock. Do not add a lock around candidate files.
Identical resumable temps publish as one transaction.

Co-authored-by: cyre <Lawrence@bettermind.ph>
cursoragent and others added 2 commits September 26, 2026 01:16
Recovery must see a complete JSONL generation. has_jsonl_timestamp holds
LOCK_EX for the scan and does not create a missing episodic file. A read
OSError still propagates so a resumable temp is kept.

Co-authored-by: cyre <Lawrence@bettermind.ph>
append_jsonl stays strict so learn.py can fail closed. post_execution and
on_failure catch OSError, warn on stderr, and return the entry they built.

Co-authored-by: cyre <Lawrence@bettermind.ph>
cursor Bot pushed a commit to diazMelgarejo/agentic-stack that referenced this pull request Sep 26, 2026
The same handoff file now lists the three fix commits on the stacked
branch, including the hook catch that landed there instead of on PR codejunkie99#71.
Both plans point at those SHAs so the next agent does not rebuild them.

Co-authored-by: cyre <Lawrence@bettermind.ph>
Comment thread .agent/tools/learn.py
Comment on lines +238 to +240
_resume_temp(leftovers[0], path)
for temp_path in leftovers[1:]:
_remove_or_raise(temp_path)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Distinct candidates can be discarded. If two staging calls produce temps with the same candidate ID but different claims or reviewer sources, they share a mirror timestamp. Recovery treats that timestamp as proof that the candidates are identical, publishes the first temp by filename, and deletes the other. A caller can receive a successful stage result even though its candidate content was discarded.

Prompt To Fix With AI
This is a comment left during a code review.
Path: .agent/tools/learn.py
Line: 238-240

Comment:
**Distinct candidates can be discarded.** If two staging calls produce temps with the same candidate ID but different claims or reviewer sources, they share a mirror timestamp. Recovery treats that timestamp as proof that the candidates are identical, publishes the first temp by filename, and deletes the other. A caller can receive a successful stage result even though its candidate content was discarded.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptileai fixed this in a01fd1a

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified in a01fd1a30e9f9cdae380645c6e628a68ac3e5b55: recovery now compares complete loaded temp payloads, not just their shared evidence timestamp. Distinct claims or reviewers raise an ambiguity error and leave every temp intact, while only identical resumable temps are collapsed and published once. The regression coverage exercises the shared-timestamp/different-reviewer case. This resolves the finding.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

File order is the canonical order. Blank lines and corrupt lines
are skipped. A row with an empty timestamp does not count.
"""
for line in raw.decode("utf-8").splitlines():

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Malformed bytes block staging. If an older episodic entry contains a non-UTF-8 byte, decoding the whole log raises UnicodeDecodeError before the corrupt-line handler runs. Manual staging now scans that log, so an unrelated malformed entry stops staging with an uncaught traceback and no candidate is published.

Suggested change
for line in raw.decode("utf-8").splitlines():
for line in raw.decode("utf-8", errors="replace").splitlines():
Prompt To Fix With AI
This is a comment left during a code review.
Path: .agent/harness/hooks/_episodic_io.py
Line: 59

Comment:
**Malformed bytes block staging.** If an older episodic entry contains a non-UTF-8 byte, decoding the whole log raises `UnicodeDecodeError` before the corrupt-line handler runs. Manual staging now scans that log, so an unrelated malformed entry stops staging with an uncaught traceback and no candidate is published.

```suggestion
    for line in raw.decode("utf-8", errors="replace").splitlines():
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Codex

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greptileai fixed this in 91c3dff

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed fixed in 91c3dff09094b35ef04f9020989ea77d28e7f420. _first_matching_action now parses each JSONL line independently via _json_object_from_line, skips non-UTF-8/corrupt lines without replacement decoding, and the regression test covers a non-UTF-8 line before a valid mirror row. This resolves the malformed-byte staging failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants