Skip to content

Fix structural ledger corruption handling and newline separation - #67

Merged
dfeen87 merged 1 commit into
mainfrom
fix-governance-ledger-corruption-handling-7124355643454311203
Aug 21, 2026
Merged

Fix structural ledger corruption handling and newline separation#67
dfeen87 merged 1 commit into
mainfrom
fix-governance-ledger-corruption-handling-7124355643454311203

Conversation

@dfeen87

@dfeen87 dfeen87 commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Ensure non-object JSON lines in verdict ledgers raise LedgerError and report findings during verification, and prepend missing newlines when appending to ledger files.


PR created automatically by Jules for task 7124355643454311203 started by @dfeen87

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@dfeen87
dfeen87 merged commit af4b28c into main Aug 21, 2026
2 checks passed
@dfeen87
dfeen87 deleted the fix-governance-ledger-corruption-handling-7124355643454311203 branch August 21, 2026 15:14
@flamehaven01

Copy link
Copy Markdown
Contributor

Your fix landed before mine did — merged and correct on all four points. I had pushed an independent one to #66 at about the same time; rather than propose it, I cross-checked yours against it.

The eight controls I wrote independently all pass against your implementation. Grafted onto origin/main at 9ffc679:

8 passed, 26 deselected

Including the ones aimed at cases beyond the two reported: "a string", 42, null as ledger lines; the CLI gate reporting line 1 instead of a traceback; and an unforeseen exception injected into validate() to confirm verify() no longer reads as clean. Two people fixing the same defect from different starting points and landing on the same behaviour is better evidence than either fix alone.

One small measured note, entirely optional — a genuine tradeoff, not a defect.

_ends_with_newline() opens the ledger a second time on every append after the first. The tail-seek design in append() exists to avoid exactly that read-open, and on Windows with real-time AV it is not cheap:

200 appends, 5 runs each, counting opens of the ledger file

                                    read-opens   ms/append (median)
_ends_with_newline()                       200         9.775
terminator reported by _tail_line            0         0.681

Your fix keeps the O(1) property — I expected the cost to scale with ledger size and measured that it does not. It is flat at ~9 ms whether the ledger holds 0 rows or 1,200:

prior rows file _ends_with_newline alternative
0 115 KB 9.642 0.711
100 173 KB 9.775 0.681
400 345 KB 9.890 0.736
1,200 805 KB 10.470 0.748

So it is a constant ~9 ms per append, not a scaling problem, and it is almost certainly a Windows-with-AV artifact — on Linux CI an extra open is microseconds and none of this is visible. For your 139-verdict suite it is around a second on a Windows dev machine and nothing at all in CI.

If you want it back, _tail_line already has the bytes in hand and can report the terminator for free — f7be307 on my fork has it, along with those eight controls if any are useful. Take the tests, take the tail change, or take neither; yours is correct as merged and I would not churn working code over 9 ms.

Thank you for the review. The first finding is the failure mode the module exists to prevent, and it shipped in the module that prevents it — I have fixed both upstream in the internal tool this was adapted from, with your two cases as the regression controls.

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