Skip to content

Handle UTF-8 GitHub merge evidence - #682

Merged
justin808 merged 4 commits into
mainfrom
jg-codex/issue-351-utf8-gate
Sep 6, 2026
Merged

Handle UTF-8 GitHub merge evidence#682
justin808 merged 4 commits into
mainfrom
jg-codex/issue-351-utf8-gate

Conversation

@justin808

@justin808 justin808 commented Sep 2, 2026

Copy link
Copy Markdown
Member

Why

The autonomous merge gate reads JSON from GitHub subprocesses. Under an ASCII locale, valid UTF-8 payloads can be tagged as US-ASCII and crash parsing. Invalid bytes can also escape the evidence boundary instead of producing the gate's structured UNKNOWN verdict.

Fixes #351.

What changed

  • Decode GitHub REST and current-integration GraphQL output as UTF-8 before JSON parsing.
  • Reject invalid UTF-8 through the existing evidence errors, so autonomous-merge-eligibility emits a structured UNKNOWN verdict.
  • Add regressions for valid non-ASCII payloads under an ASCII default encoding and invalid bytes in both evidence paths.

How to review and verify

  1. Review the byte handling in AutonomousMergeEvidence.gh_api and CurrentIntegrationEvidence.github_snapshot.
  2. Confirm the focused tests cover valid UTF-8 under an ASCII locale and invalid-byte fail-closed behavior through the CLI.

Test plan

Agent details

Commands and results

  • The three focused Ruby suites and focused RuboCop passed; the exact-head reconciliation receipt is Handle UTF-8 GitHub merge evidence #682 (comment).
  • Hosted Validate, Lint, and Claude review all passed at 089bc76c80687b7af326a9f1956f9134428b3e2e using the runs linked in Test plan.
  • The independent checker found the remediated implementation clean at ac4841b2234e88947b8fc8a7cb796c4fb51e26fa: Handle UTF-8 GitHub merge evidence #682 (comment). The final head then integrated current main, reran the focused suites, and received a clean exact-head hosted Claude review.
  • Local bin/validate was not run by lane instruction; exact-head hosted Validate is authoritative and passed.

Exact-head and replay evidence

  • Accepted and current base: f15211f985e30a85fd846c8ebc37c94bc22b90b7.
  • Published implementation head: 089bc76c80687b7af326a9f1956f9134428b3e2e.
  • GitHub reports the PR mergeable with mergeStateStatus: CLEAN; canonical exact-head CI readiness is READY.
  • TDD negative controls reproduced the ASCII-locale parse error, invalid raw-byte acceptance, and escaped-lone-surrogate failure before remediation. All cases fail closed through the structured evidence errors after the fix.

QA Evidence

  • QA lane: codex-m1-issue351-checker; independent review completed clean at the remediated implementation head, followed by exact-head focused checks and hosted Claude review at the final head; no active coordination claim remains.
  • Scope checked: both GitHub JSON readers directly used by autonomous merge evidence, recursive decoded-string validation, and CLI structured-UNKNOWN behavior.
  • Tested at: 089bc76c80687b7af326a9f1956f9134428b3e2e.
  • Automated checks: the three focused Ruby suites and focused RuboCop passed; exact-head hosted Validate, Lint, and Claude review passed using the links in Test plan.
  • Manual checks: not applicable; automated Ruby subprocess fixtures cover this helper behavior.
  • User-visible UI change: no.
  • Visual evidence: not applicable; no UI change.
  • Interaction change: no; no interactive surface changed.
  • Interaction evidence: not applicable; no interaction change.
  • Visual fix: no; no visual surface changed.
  • Negative control: observed non-visual failures for ASCII-locale parsing, invalid raw bytes, and escaped lone surrogates before the fix; focused regressions pass after the fix.
  • Performance evidence: not applicable; the change adds bounded encoding validation at existing JSON subprocess boundaries.
  • Findings: none in the scoped change; the sibling offline calibration reader is tracked separately by issue Follow-up: calibration GitHub reader lacks the UTF-8 and decoded-surrogate gates added in #682 #698 and draft PR Follow-up: calibration GitHub reader needs UTF-8 and surrogate gates #700.
  • QA required: yes.
  • QA required rationale: merge-gate evidence handling required independent review plus exact-head focused and hosted validation.
  • QA lane status: satisfied.
  • Release-blocking status: clear.
  • Process-gap disposition: script.

Coordination and reviewer telemetry

  • Batch/lane: aw-medium-wave3-20260901 / issue351-utf8-gate.
  • Thread: aw-351-m5-utf8-gate.
  • Maker: codex-m5-issue351-utf8-gate; checker: codex-m1-issue351-checker.
  • Claim instance: 94911eac-f520-4f85-9cd3-611d96853480; holder and branch matched before commit and push.

Decision log

Merge confidence

Ready for review at exact head 089bc76c80687b7af326a9f1956f9134428b3e2e: focused checks and all hosted checks are green, the exact-head Claude review produced no findings, and there are zero unresolved review threads. Merge authority remains ask; the strict security preflight requires acknowledgment of 682:high-risk-files before merge. This metadata closeout does not merge the PR.

Audit receipts

No completed-batch audit applies before merge.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 16 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 241b07ae-447a-448b-a216-569322c5e82a

📥 Commits

Reviewing files that changed from the base of the PR and between f15211f and 089bc76.

📒 Files selected for processing (5)
  • skills/pr-batch/bin/autonomous-merge-eligibility-test.rb
  • skills/pr-batch/bin/autonomous-merge-evidence-test.rb
  • skills/pr-batch/bin/current-integration-evidence-test.rb
  • skills/pr-batch/lib/autonomous_merge_evidence.rb
  • skills/pr-batch/lib/current_integration_evidence.rb

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@justin808 justin808 added the agent-claimed Active agent coordination claim; reconciled from private backend label Sep 2, 2026
@justin808

Copy link
Copy Markdown
Member Author

Independent exact-head review — BLOCKING

Head: 0d6fada3163e7bb4cca3fbee05b6874ef2871b38

Focused evidence: The three permitted suites pass at this head: 107 runs, 1,075 assertions, 0 failures/errors/skips. A base/head probe reproduces the reported ASCII-locale failure on accepted main and confirms this head accepts valid UTF-8. Raw invalid and truncated multibyte input now fails closed.

Blocking finding: The post-parse graph is not recursively checked. A byte-valid JSON response containing an escaped lone surrogate ("\udcff") parses to an invalid UTF-8 Ruby string. AutonomousMergeEvidence.gh_api returns that string; CurrentIntegrationEvidence.github_snapshot accepts it in an ignored field; and an invalid candidate OID reaches candidate_from_snapshot and raises uncaught ArgumentError. Full CurrentIntegrationEvidence.collect reproduces that non-domain exception, while the CLI rescues only CurrentIntegrationEvidence::Error and KeyError, so issue #351's required structured UNKNOWN is not guaranteed. This is a lost parse-proof/type-boundary defect. Validate every decoded key/value string recursively immediately after JSON.parse, map failure to each library's evidence error, and add escaped-surrogate CLI regressions.

Current main: origin/main=db989c34d6910095a066beef07b67fb6be657c94; it is the direct parent (0 behind/1 ahead), and git diff --check is clean. Merge compatibility is clean.

Hosted results: Validate's 299-character prompt-headroom failure is byte-for-byte the accepted-main failure and is covered by open, green PR #662; it is not PR-specific. Claude initialized but returned is_error:true before any review turn and posted no findings; that is a service/action failure. Lint passes. These distinctions do not waive the code blocker.

Unresolved review threads: 0.

Verdict: BLOCKING. Next writer action: add recursive decoded-string UTF-8 validation in both readers, add the escaped-surrogate regressions, publish a new head, and rerun focused/hosted checks plus independent exact-head review.

@justin808 justin808 added agent-claimed Active agent coordination claim; reconciled from private backend and removed agent-claimed Active agent coordination claim; reconciled from private backend labels Sep 2, 2026
The UTF-8 gate added earlier on this branch checked only the raw response
bytes. A byte-valid JSON response can still decode to an invalid Ruby
string: `"\udcff"` is well-formed JSON, passes `valid_encoding?` on the
wire, and `JSON.parse` turns it into a UTF-8 string holding an unpaired
surrogate.

That string then flowed past both readers. `AutonomousMergeEvidence.gh_api`
returned it, and `CurrentIntegrationEvidence.github_snapshot` accepted it
in a field it does not inspect, so an invalid candidate OID reached
`candidate_from_snapshot`, where `String#match?` raised an uncaught
`ArgumentError: invalid byte sequence in UTF-8`. The eligibility CLI
rescues only `CurrentIntegrationEvidence::Error` and `KeyError`, so the
structured `UNKNOWN` that issue #351 requires was not guaranteed.

Validate the decoded graph recursively over every key and value
immediately after `JSON.parse`, and map failure to each library's own
evidence error so the CLI's existing rescue paths produce `UNKNOWN`.
Checking after parse rather than at each use keeps the guard at the trust
boundary, so no later reader has to re-prove encoding.

Regressions cover both libraries directly and both CLI paths end to end.
Without the guard the CLI test reproduces the raw `ArgumentError` escaping
from `candidate_from_snapshot`.

Scope stays narrow. `autonomous_merge_calibration.rb` parses GitHub JSON
through a separate offline calibration path that never had the raw-bytes
gate either; it is outside this branch's decision path and is left for a
follow-up.

Refs #351

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9JmkTGMTLBJUXUzLr2MM3
@justin808

Copy link
Copy Markdown
Member Author

Remediation of the blocking exact-head finding — head ac4841b2234e88947b8fc8a7cb796c4fb51e26fa

The blocking finding is fixed. The post-parse graph is now validated recursively
in both readers, so a decoded lone surrogate produces a structured UNKNOWN
instead of an uncaught ArgumentError.

Fix. After JSON.parse, both AutonomousMergeEvidence.gh_api and
CurrentIntegrationEvidence.github_snapshot walk the decoded graph and check
valid_encoding? on every key and every value string, mapping failure to their
own evidence error (CollectionError / Error) so the eligibility CLI's
existing rescue paths yield UNKNOWN. Validating once at the trust boundary
rather than at each use means no later reader has to re-prove encoding.

Reproduction before the fix. With the two libraries reverted to 0d6fada3
and the new tests kept, the CLI regression reproduces the reported defect
verbatim:

/current_integration_evidence.rb:221:in 'String#match?': invalid byte sequence in UTF-8 (ArgumentError)
    from CurrentIntegrationEvidence.candidate_from_snapshot
    from CurrentIntegrationEvidence.collect
    from skills/pr-batch/bin/autonomous-merge-eligibility:314:in '<main>'

The library-level regressions fail the same way (CollectionError expected but nothing was raised, Error expected but nothing was raised), confirming these
are real red-to-green tests and not assertions that already held.

Focused tests at this head. All three permitted suites green:

suite runs assertions F/E/S
autonomous-merge-eligibility-test.rb 62 826 0/0/0
current-integration-evidence-test.rb 27 121 0/0/0
autonomous-merge-evidence-test.rb 22 144 0/0/0
total 111 1091 0/0/0

That is +4 runs and +16 assertions over the 107/1075 baseline recorded at
0d6fada3. RuboCop is clean on all five changed files.

Full bin/validate was deliberately not run: #662 is still open, so a full run
would report the known prompt-headroom failure rather than anything about this
change.

Base. origin/main is db989c34d6910095a066beef07b67fb6be657c94, still the
direct parent — 0 behind, 1 ahead before this commit. git diff --check clean.

Hosted checks at the exact head. Lint: success, claude-review: success, validate: failure.

claude-review was a service failure at 0d6fada3 (initialized, then
is_error:true with no review turn); it ran clean at this head. The single
validate failure is byte-for-byte the accepted-main failure:

FAIL: expected installed prompt guard to pass, got 1: FAIL: pr-batch Codex goal prompt template has 299 chars of headroom, must keep at least 300

It is the pre-existing prompt-headroom failure covered by open PR #662, not
PR-specific, and every other step in that run passed.

Review threads. 0 unresolved, and 0 review threads exist on this PR — the
blocking finding was posted as an issue comment, so there is no thread to
resolve. Nothing has been resolved or dismissed.

Out of scope, filed separately. autonomous_merge_calibration.rb parses
GitHub JSON through its own client that has neither the raw-bytes gate nor the
decoded-graph gate, and calls String#match? on parsed author and SHA values.
Same defect class, different code path, never covered by this branch. Filed as
#698 rather than widened into this remediation; it is offline calibration, not a
merge gate.

Next action. Independent exact-head review at ac4841b2.

@justin808 justin808 added agent-claimed Active agent coordination claim; reconciled from private backend and removed agent-claimed Active agent coordination claim; reconciled from private backend labels Sep 2, 2026
@justin808

Copy link
Copy Markdown
Member Author

Independent exact-head review of ac4841b2234e88947b8fc8a7cb796c4fb51e26fa against base db989c34d6910095a066beef07b67fb6be657c94 for PR #682.

Scope: reviewed the five-file diff, verified the live PR head matches the immutable head, and ran the three focused evidence suites plus bounded raw-byte / escaped-surrogate probes on head and base.

Evidence: all three exact-head suites passed (62/826, 27/121, 22/144). Base-parent probes reproduced invalid decoded strings for escaped-surrogate payloads; the exact head rejects them with structured CollectionError / Error. Raw invalid UTF-8 stays fail-closed on both evidence paths.

Unresolved review threads: 0.

Verdict: CLEAN.

@justin808 justin808 added agent-claimed Active agent coordination claim; reconciled from private backend and removed agent-claimed Active agent coordination claim; reconciled from private backend labels Sep 2, 2026
* origin/main:
  Deduplicate batch-title contracts and restore prompt headroom (#662)
@justin808

Copy link
Copy Markdown
Member Author

Current-head reconciliation complete on 089bc76c80687b7af326a9f1956f9134428b3e2e.

  • merged verified origin/main f15211f985e30a85fd846c8ebc37c94bc22b90b7
  • focused tests passed: autonomous-merge-eligibility-test.rb, autonomous-merge-evidence-test.rb, current-integration-evidence-test.rb
  • trusted hosted QA scan on the exact base returned NOT_APPLICABLE
  • bin/lint passed
  • unresolved review threads: 0

Branch pushed to jg-codex/issue-351-utf8-gate.

@justin808 justin808 removed the agent-claimed Active agent coordination claim; reconciled from private backend label Sep 2, 2026
@justin808
justin808 marked this pull request as ready for review September 3, 2026 03:17
Comment thread skills/pr-batch/lib/current_integration_evidence.rb
Comment thread skills/pr-batch/lib/autonomous_merge_evidence.rb
Comment thread skills/pr-batch/lib/current_integration_evidence.rb

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 089bc76c80

ℹ️ 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".

Comment thread skills/pr-batch/lib/autonomous_merge_evidence.rb
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-06T10:03:06.242886Z a713388 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@justin808 justin808 added the agent-claimed Active agent coordination claim; reconciled from private backend label Sep 6, 2026
Comment thread skills/pr-batch/lib/current_integration_evidence.rb
@claude

claude Bot commented Sep 6, 2026

Copy link
Copy Markdown

Review summary

Focused, well-targeted fix for issue #351. I reviewed the two library changes (AutonomousMergeEvidence.gh_api, CurrentIntegrationEvidence.github_snapshot) and the new regression tests.

Correctness

  • The fix addresses two distinct failure modes correctly:
    1. Raw GitHub subprocess bytes getting mis-tagged as US-ASCII under a non-UTF-8 locale — fixed by force_encoding(Encoding::UTF_8) + valid_encoding? before JSON.parse, which is the right place since that's where the original crash occurred (not after, in downstream consumers).
    2. JSON containing lone UTF-16 surrogate escapes (e.g. \udcff) that are syntactically valid JSON but decode to Ruby strings with invalid byte sequences — fixed by the new recursive decoded_json_strings_valid? walk over the parsed structure, applied immediately after JSON.parse in both readers.
  • Order of checks is correct in both call sites: raw-byte validation → parse → decoded-string validation → use. This closes the gap where candidate_from_snapshot's String#match? could previously raise an uncaught ArgumentError on an invalid-but-JSON-legal string.
  • Error classes are chosen correctly so existing CLI rescue blocks (AutonomousMergeEvidence::CollectionError, CurrentIntegrationEvidence::Error) turn these into the structured UNKNOWN verdict rather than an uncaught exception — verified this against autonomous-merge-eligibility's rescue clauses.
  • paginate in autonomous_merge_evidence.rb routes through gh_api, so every paginated call (files, commits, reviews, comments, timeline) gets the same guard, not just the two direct api.call sites — good coverage, not just a narrow patch.
  • Valid non-ASCII text (raw UTF-8 bytes, and well-formed surrogate pairs that decode to a single valid codepoint) is correctly left alone — no false positives from the new guard.

Security

  • This is a pure hardening/fail-closed change: unrecoverable byte sequences from an external process (GitHub CLI output) now reliably produce a structured UNKNOWN decision instead of an uncaught ArgumentError propagating out of the merge-gate evidence path. That's the right direction for an autonomous-merge safety gate — no new attack surface introduced.

Tests

  • New regressions cover both libraries directly (ASCII-locale valid UTF-8, invalid raw bytes, decoded lone surrogates in both value and key position) and thread the same cases through the CLI end-to-end (autonomous-merge-eligibility-test.rb) to confirm the UNKNOWN verdict surfaces correctly. This is thorough — it tests at the unit level and at the integration/CLI boundary.

Minor nit (posted inline)

  • decoded_json_strings_valid? is duplicated verbatim between autonomous_merge_evidence.rb and current_integration_evidence.rb. Worth extracting to a shared helper at some point, not blocking.

Scope

No blocking issues found.

@justin808
justin808 merged commit cb1790f into main Sep 6, 2026
16 checks passed
@justin808
justin808 deleted the jg-codex/issue-351-utf8-gate branch September 6, 2026 22:17
justin808 added a commit that referenced this pull request Sep 7, 2026
…ordination-observability-recovery

* origin/main:
  Load workflow skill instructions by stage and document Astra host capabilities (#783)
  Authenticate closed-unmerged verification PR artifacts (#771)
  Archive public technical implementation plans (#689)
  Centralize Astra routing and add behavioral evaluation support (#759)
  Document attended and overnight operator workflow (#572)
  Fix issue-targeted completed-batch publication (#439)
  Handle UTF-8 GitHub merge evidence (#682)
  Enforce process-gap mechanism targets (#681)
  Clarify flaky CI routing in replicate-ci (#622)
  Fix recorded CI base binding for validated integration reuse (#779)
  Bump anthropics/claude-code-action from 1.0.210 to 1.0.215 (#780)
  Raise the workflow byte caps until the #392 extraction lands (#776)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

# Conflicts:
#	skills/address-review/SKILL.md
#	skills/plan-pr-batch/SKILL.md
#	skills/pr-batch/SKILL.md
justin808 added a commit that referenced this pull request Sep 7, 2026
…-marker-sync

* origin/main:
  Load workflow skill instructions by stage and document Astra host capabilities (#783)
  Authenticate closed-unmerged verification PR artifacts (#771)
  Archive public technical implementation plans (#689)
  Centralize Astra routing and add behavioral evaluation support (#759)
  Document attended and overnight operator workflow (#572)
  Fix issue-targeted completed-batch publication (#439)
  Handle UTF-8 GitHub merge evidence (#682)
  Enforce process-gap mechanism targets (#681)
  Clarify flaky CI routing in replicate-ci (#622)
  Fix recorded CI base binding for validated integration reuse (#779)
  Bump anthropics/claude-code-action from 1.0.210 to 1.0.215 (#780)
  Raise the workflow byte caps until the #392 extraction lands (#776)
  Classify GitHub intake lookup failures safely (#673)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
justin808 added a commit that referenced this pull request Sep 7, 2026
…batch-controls-followup

* origin/main:
  Clarify revert runbook closure scope and release examples (#704)
  Strengthen TDD guidance with falsifiable behavioral tests (#676)
  Load workflow skill instructions by stage and document Astra host capabilities (#783)
  Authenticate closed-unmerged verification PR artifacts (#771)
  Archive public technical implementation plans (#689)
  Centralize Astra routing and add behavioral evaluation support (#759)
  Document attended and overnight operator workflow (#572)
  Fix issue-targeted completed-batch publication (#439)
  Handle UTF-8 GitHub merge evidence (#682)
  Enforce process-gap mechanism targets (#681)
  Clarify flaky CI routing in replicate-ci (#622)
  Fix recorded CI base binding for validated integration reuse (#779)
  Bump anthropics/claude-code-action from 1.0.210 to 1.0.215 (#780)
  Raise the workflow byte caps until the #392 extraction lands (#776)
  Classify GitHub intake lookup failures safely (#673)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

# Conflicts:
#	skills/plan-pr-batch/SKILL.md
#	skills/pr-batch/SKILL.md
#	skills/pr-batch/bin/autonomous-merge-eligibility-test.rb
justin808 added a commit that referenced this pull request Sep 7, 2026
…ollowups

* origin/main: (21 commits)
  Make cross-runner blocker owner routes navigable (#680)
  Separate draft-head CI from merge-ready validation (#701)
  Refresh workflows through native host CLIs (#321)
  Fix writing-style resolver fallback for split pack roots (#736)
  Collapse the closing stack into one terminal structure for small single-repo batches (#707)
  Normalize known terminal coordination state casing in batch receipts (#786)
  Harden durable evidence replay edge cases (#269)
  Preserve omitted check-run actions in optional CI hold evidence (#768)
  Clarify revert runbook closure scope and release examples (#704)
  Strengthen TDD guidance with falsifiable behavioral tests (#676)
  Load workflow skill instructions by stage and document Astra host capabilities (#783)
  Authenticate closed-unmerged verification PR artifacts (#771)
  Archive public technical implementation plans (#689)
  Centralize Astra routing and add behavioral evaluation support (#759)
  Document attended and overnight operator workflow (#572)
  Fix issue-targeted completed-batch publication (#439)
  Handle UTF-8 GitHub merge evidence (#682)
  Enforce process-gap mechanism targets (#681)
  Clarify flaky CI routing in replicate-ci (#622)
  Fix recorded CI base binding for validated integration reuse (#779)
  ...

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-claimed Active agent coordination claim; reconciled from private backend

Projects

None yet

Development

Successfully merging this pull request may close these issues.

autonomous-merge-eligibility crashes instead of emitting UNKNOWN when LANG/LC_ALL are unset and the PR payload has non-ASCII

1 participant