fix(noema): fail closed on malformed LLM JSON instead of crashing - #1507

…e draft-gate fix

main's opencode-review.yml has been substantially redesigned since this
branch last synced (#1507/#1532): the old 325-minute synchronous poll
loop is gone, replaced by a fast check-once-dispatch-and-fail-closed
"Resolve current-head formal OpenCode verdict" step plus a separate
formal-receipt "wake" callback that reruns the failed job once a verdict
actually lands, instead of blocking a runner for hours. #1533's
head_sha warn-and-proceed design (which an earlier revision of this
fix's blob-pin port matched) was also reverted upstream (#1540, real
bugs found by Codex/Devin) -- restored to the original hard-fail
assertions and current blob pin.

The draft-gate exemption itself is unaffected by any of that and is
re-applied cleanly against the new three-step structure:
- "Resolve current-head formal OpenCode verdict" now exits early with
  verdict=DRAFT for a draft PR, mirroring its existing closed exit.
- "Request current-head OpenCode review execution"'s own if: also skips
  drafts, so a transient OIDC/dispatch failure can't turn a draft PR's
  check red before the exemption runs.
- The now-trivial "Fail closed without a current-head OpenCode verdict"
  step (no gh calls or loop left in it at all) treats VERDICT=DRAFT the
  same as VERDICT=CLOSED.
- converted_to_draft added to the trigger types, so a ready PR converted
  back to draft with no new commit still gets a fresh run.

tests/test_opencode_required_verdict_regression.py's old _run_step
helper and its six tests assumed the removed monolithic polling step;
replaced with _run_verdict_step/_run_fail_closed_step matching the new
split, keeping the same draft/closed/ready-for-review coverage.

Full suite: 2216 passed, 1 skipped, 21 subtests. Ruff, interrogate,
YAML, and shell-syntax checks clean.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4

…alwisdomlab-commercialization-afow1j

Reconciles this branch's fix for noema_review_gate.py's hardcoded 120s
timeout (NOEMA_LLM_REQUEST_TIMEOUT_SECONDS=10800 + shared
NOEMA_LLM_TOTAL_BUDGET_SECONDS=19800 deadline across the initial call
and its one possible repair call) with main's independent fix for the
exact same bug (PR #1507, NOEMA_LLM_TIMEOUT_SECONDS=14400 flat), plus
main's several other genuine, unrelated hardening fixes in the same
file: fail-closed handling of malformed/deeply-nested LLM JSON,
UnicodeDecodeError safety, and skipping the repair-retry request when
the PR head has moved (new expected_head parameter and
StaleHeadDuringRepairRetryError).

Kept main's JSON-crash and stale-head logic entirely intact, and kept
this branch's shared-deadline design for the timeout itself: a flat
4-hour constant applied independently to both the initial and repair
calls could sum past the 6-hour GitHub-hosted job execution ceiling,
which the shared budget already accounts for. Removed the now-unused
NOEMA_LLM_TIMEOUT_SECONDS constant and updated its references.

Fixed 3 additional breaks the merge surfaced in this branch's own
tests: two calls to call_llm missing the newly-required expected_head
argument, and one test asserting a stale literal 14400 timeout value.
Full suite: 2211 passed, 1 skipped, coverage 100%, interrogate 100%.

)

* fix(noema): fail closed on malformed LLM JSON instead of crashing

The required noema-review check on contextual-orchestrator#960 crashed
with an unhandled json.JSONDecodeError inside extract_json_object,
called from call_llm. A truncated or malformed model reply (observed:
"Expecting property name enclosed in double quotes") propagated past
every layer up to the module's `except RuntimeError` guard, which
only catches RuntimeError, so the whole job died with a raw traceback
instead of a readable review-blocked signal.

Convert the json.JSONDecodeError into the same fail-closed
RuntimeError this file already raises for its other "no usable
verdict" cases in call_llm (unsupported decision, missing summary,
malformed finding) -- no new failure path invented, just reusing the
existing one. The message embeds the raw model response, scrubbed of
secrets and bounded to MAX_LLM_RESPONSE_LOG_CHARS, so the job log
still shows why the verdict was unusable. Also make the top-level
__main__ handler print `::error::{exc}` instead of a bare message,
matching this repo's convention in sibling CI gates (e.g.
opencode_review_receipt_gate.py, select_nvidia_nim_model.py).

Adds regression tests reproducing the exact crash signature at both
the extract_json_object unit level and the call_llm integration
level, asserting a clean RuntimeError propagates instead of an
unhandled JSONDecodeError.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* docs(gaps): record noema-review JSON-crash fail-closed fix (PR #1507)

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix(noema): repair malformed verdict JSON once

* fix(ci): bound required-workflow-bootstrap awk extraction to its own job

The exact-head-path-policy quick gate's assertion for the required-workflow
bootstrap job used an awk range pattern
(/^  required-workflow-bootstrap:$/,/^[^ ]/) whose end pattern never matches
because no job key in opencode-review.yml starts at column 0. That swept an
unrelated `if:` line from a different job into the extracted block and
produced a false assertion failure.

Same root cause already diagnosed and fixed on main in #1506; ported the
identical one-line awk fix here since this branch forked before that fix
landed. Uses an explicit state flag so the end pattern is only tested
starting on the line after the start match.

bash scripts/ci/test_strix_quick_gate.sh: FAIL -> PASS
coverage run -m pytest tests -q && coverage report --show-missing: 2129
passed, 1 skipped, 21 subtests; 100% on scripts/ci/
interrogate: 100%

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix(noema): allow long orchestrated reviews

* fix(noema): stop logging raw LLM output, fail closed on malformed envelopes

Devin Review found two issues in this PR's own prior fail-closed fix:

Security (priority): extract_json_object's malformed-JSON diagnostic
embedded the LLM's raw response, scrubbed only through a finite,
pattern-based regex list (SENSITIVE_DATA_SCRUB_PATTERNS). noema-review.yml
is a pull_request_target workflow with public Actions logs, and an LLM can
echo back or hallucinate a credential in a shape those patterns don't
recognize. No regex allowlist of known secret shapes can close that gap, so
the fix stops trying to: the diagnostic now logs only a content length and
a truncated SHA-256 fingerprint, never the raw or scrubbed text.
MAX_LLM_RESPONSE_LOG_CHARS is removed as unused.

Bug: call_llm parsed the raw HTTP envelope (json.loads + four chained
.get()/[0] accesses) before the try block that feeds the #1504 one-time
repair-retry, so a non-JSON body or a wrong-shaped envelope (non-object
top-level JSON, non-list choices, non-object choices[0]/message, non-string
content) crashed with an unhandled exception before ever reaching the
verdict-JSON repair boundary. New extract_llm_message_content() validates
each step explicitly with isinstance checks (never a broad except, so real
bugs still surface) and now runs inside the existing repair-retry try
block, so a malformed envelope gets the same one repair attempt a
malformed verdict already gets before failing closed.

Regression tests: extended test_extract_json_object_fails_closed_on_malformed_json
to assert an unrecognized-shape credential (and a known-shape one) never
appears in the new diagnostic; added direct branch coverage for every
extract_llm_message_content failure mode plus call_llm integration tests
for the repair-once and exhausted-repair envelope paths.

2151 tests pass; 100% coverage (branch included) and 100% docstring
coverage on scripts/ci/.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix(noema): decode non-UTF-8 gateway replies inside the repair boundary

Devin Review's third pass on PR #1507 found one more crash-before-repair
instance in call_llm: response.read().decode("utf-8") ran before the try
block that feeds the one-time schema-repair retry, so a gateway reply
containing invalid UTF-8 bytes raised an unhandled UnicodeDecodeError
instead of getting the same repair-then-fail-closed treatment every other
malformed-envelope shape already gets.

New decode_llm_response_body(raw_bytes) converts a UnicodeDecodeError into
the same bounded RuntimeError call_llm already uses elsewhere, called from
inside the existing repair-retry try block. Per the round-2 security fix,
the diagnostic never embeds the raw response bytes (even the undecodable
fragment) -- only a length and a truncated SHA-256 fingerprint, since a
body containing invalid UTF-8 could still contain a credential-adjacent
byte sequence.

Regression tests: direct unit coverage of decode_llm_response_body (happy
path plus the fail-closed path, asserting no secret-shaped or tail content
leaks into the message), and a call_llm integration test proving one
repair-retry request followed by a clean top-level RuntimeError when both
the first and retry responses contain invalid UTF-8.

Also verified, no code change needed, per this round's two informational
notes: repair recursion stays bounded to one retry (if repair_error: raise
prevents further recursion), and a falsey-but-wrong-shaped envelope field
(choices/message/content) still fails closed one layer down in
extract_json_object even though extract_llm_message_content treats it
leniently as absent.

2154 tests pass; 100% coverage (branch included) and 100% docstring
coverage on scripts/ci/.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix(noema-review-gate): surrogatepass the fingerprint hash to avoid UnicodeEncodeError

Devin Review finding: a malformed verdict containing an escaped lone
surrogate makes the fail-closed diagnostic's sha256(...).encode("utf-8")
raise UnicodeEncodeError before the repair-retry path runs, crashing the
required review check instead of failing closed cleanly.

Use errors="surrogatepass" on the encode call so a lone surrogate is
representable, and add a regression test reproducing the exact crash
signature pre-fix.

* fix(test): split gitleaks-flagged UUID literal via fake_secret helper

gitleaks/GHAS flagged tests/test_noema_review_gate.py:187's
unrecognized_shape_secret literal ("3f29e1a7-8b44-4c1d-9e77-2a5f9c001234")
as a Generic API Key. It is a synthetic UUID-shaped fixture -- the test
deliberately uses a credential-shaped value the finite scrub-pattern list
does NOT recognize, to prove extract_json_object still never embeds raw
content in its error message even for an unrecognized secret shape.

Per this repo's own .gitleaksignore policy ("new findings remain
blocking"), the fix is not an allowlist entry but constructing the
literal at runtime, matching this same file's existing fake_secret(*parts)
helper (already used at lines 56-57 for github_pat-shaped fixtures) so
gitleaks' static scanner sees no single matching string literal.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix(ci): remove grep -q from test_strix_quick_gate.sh pipeline checks

grep -q exits on first match and closes its end of the pipe; if the
upstream awk is still writing a large block, it gets SIGPIPE (141).
Under `set -o pipefail` that non-zero awk status wins over grep's real
0, so `if pipeline; then` sees the pipeline as failed even though grep
found a genuine match — silently missing e.g. a forbidden `if:` key or
a fenced-diff marker that should have failed the check.

Ports the same-file fix from PR #1506 to this branch's two call sites
(required-workflow-bootstrap job-block check; opencode review
REQUEST_CHANGES fenced-diff check). This branch's awk patterns were
already the corrected job-block-boundary form, so only the grep -q
removal was needed here.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix(ci): allowlist superseded historical gitleaks finding on this PR's own commit

gitleaks scans this PR's full commit range (base..head), not just the
current file content at HEAD. Commit 6657eb7 introduced a synthetic
UUID-shaped test fixture as a bare string literal; commit 3f3bb47 (already
on this branch) fixed it by constructing it at runtime via the file's
existing fake_secret(*parts) helper. The fix at HEAD is correct, but the
now-superseded intermediate commit remains reachable in the PR's history,
so gitleaks keeps re-flagging it on every scan of the full range.

Per this repo's own .gitleaksignore convention ("new findings remain
blocking" -- this is not a new finding, it's a historical instance of an
already-fixed one baked into an intermediate commit that can't be edited
without rewriting this PR's history), added a fingerprint entry matching
the file's established format and precedent for exactly this situation.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* test: pin Noema fixture ignore

* fix: bind Noema concurrency to head

* fix: reject stale Noema review runs

* test(ci): pin Noema concurrency policy prose alongside its workflow contract

test_required_pull_request_workflows_cancel_superseded_runs asserted only
noema-review.yml's own concurrency expression. docs/pr-review-and-merge-procedure.md
describes the same policy in prose (Noema's cancel-in-progress: true scoped to
one exact PR head via a head-SHA-inclusive concurrency key), but nothing pinned
that description staying in sync with the workflow -- violating this repo's own
"contract tests pin workflows AND prose" convention (CLAUDE.md).

Extended the noema-review.yml branch of the existing test to also load and
assert the matching prose. No behavior change; test-only.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* test: pin Noema head-source policy

* fix: resolve workflow-run PR head

* test: add regression coverage for the Noema stale-trigger guard fixes

A concurrent session on this branch landed the same two Devin Review
fixes independently verified here (workflow_run.head_sha reading the
base commit instead of the PR head; case-sensitive expected-head SHA
comparisons rejecting valid uppercase dispatches). This adds
complementary regression tests on top of that already-landed fix:

- tests/test_noema_orchestrator_workflow_contract.py:
  test_workflow_run_expected_head_uses_pull_request_head_not_base_commit
  and test_workflow_run_expected_head_fails_closed_when_pull_requests_is_empty
  prove, with distinct base vs. PR-head SHA values, that EXPECTED_HEAD
  now resolves to the PR head; test_stale_trigger_step_compares_expected_head_case_insensitively
  and test_stale_trigger_step_still_rejects_a_genuinely_different_head
  execute the workflow's own bash step against a fake `gh` to prove the
  case-insensitive comparison without weakening genuine stale detection.
- tests/test_noema_review_gate.py:
  test_uppercase_expected_head_is_not_stale_before_model_work and
  test_uppercase_expected_head_is_not_stale_before_publication cover
  both Python-side comparison sites end-to-end through submit_review.
- scripts/ci/noema_review_gate.py: expand inspect_and_review's docstring
  to record the case-sensitivity rationale.
- docs/product-technical-gap-baseline.md: dated entry recording both
  confirmed findings, root cause, and evidence.

100% coverage (branch included) and 100% docstring coverage on
scripts/ci/; full test suite green.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix: cancel closed PR Noema runs

* fix: scope Noema cleanup to closed PR

* fix: canonicalize Noema trigger identity

* fix: restore repo-wide status-filtered scan for Noema close cleanup

A concurrent session (e0f542f) landed a fix for both Devin Review
findings on the cancel-closed-pr-runs job while this session was
building its own; this session's pre-push rebase surfaced it before
push. Its Bug 1 fix (drop the bare head_sha match, select only by the
PR-scoped display_title) is correct and kept as-is.

Its Bug 2 fix swapped the five-status sequential sweep for one
unfiltered snapshot from `actions/workflows/noema-review.yml/runs`.
That endpoint is scoped to workflow files that exist in the target
repository's own tree; noema-review.yml runs against sibling
repositories only through the organization's required-workflow
ruleset and is never itself committed there (README.md's "siblings
call it" section), so the endpoint is not guaranteed to resolve for
the sibling-repository runs this job's cleanup exists for -- its
primary use case, not an edge case. A failure there is caught by the
job's existing fail-open handling, so it would not error; it would
silently no-op cleanup for every sibling repository. strix.yml's
sibling job, solving the identical cross-repo problem, deliberately
uses the repository-wide `/actions/runs` endpoint instead.

Restores that repository-wide, `status`-server-filtered endpoint
(bounding each query to only currently active runs, not this
workflow's entire history -- it is this org's central, highest-volume
review workflow) and replaces the original single sequential sweep
with a bounded multi-pass re-scan: minimum two full passes always (a
run missed by every status query in pass 1 has, by definition,
settled into a checkable status by pass 2), a third only when either
of the first two found something, capped at three total.

Updates e0f542f's own new jq/bash-executing test for the restored
status-filtered query shape, and adds two more of the same kind:
proving a shared head SHA across two different PRs only cancels the
closing PR's run, and proving a run that only becomes visible on a
status's second query is still cancelled. All three were confirmed to
fail against e0f542f alone before passing against this fix.

coverage run -m pytest tests: 2169 passed, 1 skipped, 21 subtests.
coverage report: 100% on scripts/ci/. interrogate: 100% docstrings.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix: allow two-hour OpenCode reviews

* fix: allow long-running orchestrator reviews

* fix(tests): re-pin review-dispatch blob SHA contract after workflow edit

The pinned REVIEW_DISPATCH_BLOB_SHA in
tests/test_pr_review_autofix_nvidia_nim_contract.py was left stale after a
concurrent commit changed .github/workflows/opencode-review-dispatch.yml's
run-timeout, breaking
test_review_dispatch_blob_sha_stays_paired_with_trusted_workflow (required
'quality' check). Updated the pin to the file's current git blob SHA.

* docs: name the reviewed blob contract

* fix: wait for multi-hour OpenCode verdicts

* fix: widen opencode-review required-verdict poller past its downstream budget

Devin Review found the "Fail closed without a current-head OpenCode verdict"
poller's 639 sleeps x 30s = 319.5 minutes of patience was less than
opencode-review-dispatch.yml's own opencode-review-target job's 325-minute
timeout-minutes budget, before even counting the dispatch/queueing delay and
the validate-pr-metadata -> coverage-source-tree -> coverage-evidence chain
that job's needs: requires first. CodeRabbit separately found the loop's
sleep calls were the only budgeted time -- the gh api --paginate calls
themselves had no timeout and could silently consume unaccounted-for time.

Investigating the full pipeline surfaced a platform ceiling neither
finding's fix could fully absorb: GitHub-hosted runners hard-cap every job's
wall-clock at 360 minutes regardless of timeout-minutes, so no poller budget
can reach the realistic ~415-430 minute worst case (upstream chain plus the
downstream job's own 325m). This fix maximizes patience within that ceiling
and documents the residual gap rather than silently leaving it unaddressed:

- Raise the poller's attempts from 640 to 661 (330m of pure-sleep patience,
  5m past the downstream job's own budget) and the enclosing job's
  timeout-minutes from 325 to 355 (5m under the 360m hard cap).
- Wrap the gh api --paginate call in `timeout 25` so one hung or heavily
  paginated call can't consume unbudgeted time; a failed/timed-out call now
  degrades to "no verdict yet" and keeps polling instead of crashing the
  step under set -euo pipefail.
- Replace the regression test's hard-coded literal assertions (640, 325)
  with ones that parse both workflows' live numbers and assert the budget
  inequalities directly, so a future edit that breaks the relationship
  fails the test instead of only an edit that changes the literal.
  Verified the new tests actually catch the original bug by temporarily
  reverting to the pre-fix numbers.
- Document the residual worst-case gap (2026-08-31 entry,
  docs/product-technical-gap-baseline.md): fully covering the realistic
  worst case needs an architecture change (splitting the wait across
  multiple short-lived dispatches) out of scope for this budget-sizing fix.

Validation: coverage run -m pytest tests -q -- 2173 passed, 1 skipped, 21
subtests; coverage report -- 100% on scripts/ci/; interrogate -- 100%
docstrings; actionlint v1.7.12 -- no findings on the modified workflow.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix: cover complete OpenCode review window

* security: bound external review resource use

* fix: check live PR head before Noema's repair-retry LLM call

CodeRabbit review on #1507: call_llm's one-time repair-retry request fired
unconditionally after a malformed first verdict, with no check that the PR
head hadn't moved since the first attempt started. inspect_and_review
already checks expected_head before model work and before publication, but
a head move mid-first-attempt could still burn a second, potentially
multi-hour NOEMA_LLM_TIMEOUT_SECONDS call for a verdict the existing
post-call check would discard anyway.

call_llm now takes expected_head and, on the repair-retry path only, re-
fetches the live PR via the existing fetch_pr helper and compares its
headRefOid (lowercased, matching inspect_and_review's existing comparisons)
before firing the retry. A mismatch raises the new
StaleHeadDuringRepairRetryError, which inspect_and_review catches and
treats as a clean skip (return 0), consistent with its other two
stale-head checks rather than a hard failure.

Adds regression tests for the skip-on-stale-head path, the unchanged
repair-on-matching-head path, and inspect_and_review's clean handling of
the new exception. Updates every existing call_llm(...) call site for the
new required parameter.

coverage run -m pytest tests -q: 2174 passed, 1 skipped, 21 subtests.
coverage report: 100% on scripts/ci/. interrogate: 100%. ruff check: clean
on touched files.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix: cancel superseded Noema model runs

* fix: guard Noema supersession with live head

* test: execute Noema supersession selector

* fix: extract balanced Noema verdict JSON

Signed-off-by: Seongho Bae <[email protected]>

* fix: fence Noema cleanup against newer runs

Signed-off-by: Seongho Bae <[email protected]>

* fix(ci): move fork-rejection closed-action check into script body

The 'Reject untrusted fork review resource consumption' step used a
YAML-level 'if: github.event.action != closed' condition, violating this
job's established convention (enforced by
scripts/ci/test_strix_quick_gate.sh) that required-workflow-bootstrap must
not depend on required-workflow event payload fields via step/job-level if:
conditionals. Moved the closed-action check into the script body as an
early exit, matching the existing pattern used elsewhere in this same job.

* fix: make Noema supersession directional

* fix: fail closed on nested Noema JSON

Signed-off-by: Seongho Bae <[email protected]>

* fix: normalize Noema decoder recursion failure

* fix(ci): mark extract_json_object's dead isinstance branch no-branch

The isinstance(candidate, dict) check's False arm is unreachable by JSON
grammar (a successful raw_decode starting at '{' can only yield a dict),
exactly as this function's own docstring already documents. It had no
coverage pragma, so the 100%-branch-coverage gate (fail_under=100) was
failing on this pre-existing, structurally-dead branch. Added
'# pragma: no branch' with a short inline explanation, matching this
repo's existing convention of documenting genuinely unreachable code
rather than fabricating an impossible test case for it.

* fix(ci): remove unreachable Noema JSON branch

* fix: guard Noema supersession's live-head re-check against transient failure

The directional cancellation guard (run IDs smaller than the current run,
plus a fresh live-head re-check immediately before each cancellation) added
to noema-review.yml's "Cancel superseded Noema runs after live-head
validation" step closed a real TOCTOU race, but its new live-head re-check
was itself an unguarded `gh api` command substitution under this step's own
`set -euo pipefail`. A transient failure on that one ancillary call (rate
limit, network blip) would exit the whole step non-zero, failing the entire
noema-review job and blocking a perfectly valid, live-head Noema review over
a housekeeping hiccup unrelated to the review itself (Devin review on

Wrap the re-check the same way every other `gh api` call in this file
already is: on failure, log a warning and exit 0 rather than propagate the
failure. Treat "cannot verify" the same as "verified stale" -- stop
cancelling further runs, but let the job, and the actual review later in
it, proceed.

Adds test_superseded_cleanup_survives_a_transient_live_head_lookup_failure,
executing the real production bash against a fake `gh` that fails only the
live-head lookup, and extends the structural concurrency test with a
docstring enumerating the four invariants this mechanism now holds together
across the multiple review rounds it took to land, plus assertions pinning
the step's pull_request_target-only gate and the now-guarded re-check.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* docs: align Noema coverage evidence

* fix(ci): reject nested Noema JSON recovery

* docs(noema): document RecursionError handling as defense-in-depth

Investigated the review-thread concern that the RecursionError-handling
regression test is synthetic (monkeypatches raw_decode) rather than using a
real deep payload, and that a real payload was previously shown accepted
without raising on the hosted Python 3.14 runner (job 99642234627, commit
ec23350: "DID NOT RAISE RuntimeError").

Verified independently:
- A real depth = max(20_000, sys.getrecursionlimit() * 2) nested-array
  payload does raise RecursionError on Python 3.11-3.13, but is decoded
  successfully (no exception) by the C-accelerated scanner on Python 3.14.7,
  confirming the hosted-runner evidence. No real payload reproduces the
  condition on this job's own runtime, so the test's monkeypatch is the
  correct choice, not a workaround.
- RecursionError is a RuntimeError subclass, so call_llm's existing
  `except RuntimeError` (already wrapping extract_json_object and every
  post-decode field check, including the decision/summary/findings reads
  the thread also asked about) already converted an unhandled
  RecursionError into the same clean fail-closed exit before this branch
  existed. The explicit `except RecursionError` clause is defense-in-depth
  for a bounded, scrubbed, fingerprinted diagnostic — not a different
  fail-closed outcome.

Documents both points in extract_json_object's docstring and the test's
docstring so a future reader does not mistake the synthetic test for
evidence of an always-reproducible crash.

* fix(ci): pass github.event.action through env in opencode-review.yml

CodeRabbit nitpick on PR #1507: two run: blocks in the required-workflow
bootstrap job still interpolated ${{ github.event.action }} directly into
their shell scripts instead of threading it through env: first, unlike the
pattern this same file already uses at line 38 and 222. Match the existing
convention at the two remaining sites (the "Wait for a current-head OpenCode
verdict" and "Fail closed without a current-head OpenCode verdict" steps).

* fix(ci): bound Noema JSON nesting depth explicitly, independent of raw_decode

Review follow-up (seonghobae, PR #1507): the recursion-handling fix
(aee49c6/b53b0b16) proved only that a raised RecursionError from
json.JSONDecoder.raw_decode is converted to the same bounded diagnostic —
it did not prove real excessive nesting fails closed on this job's own
runtime. Verified: a real depth = max(20_000, sys.getrecursionlimit() * 2)
nested-array payload does raise RecursionError on Python 3.11-3.13, but
decodes successfully with no exception at all on the Python 3.14 hosted
runner this job actually runs on (job 99642234627, commit ec23350:
"DID NOT RAISE RuntimeError" against that exact real payload). Relying on
raw_decode's own recursion behavior made the fail-closed guarantee a
property of whichever CPython version happens to run the job, not of this
function.

Adds an explicit, string-literal-aware bracket-depth scan
(_json_nesting_within_bound, MAX_JSON_NESTING_DEPTH = 100 -- generously
above the verdict schema's real ~5-level maximum) that runs before
raw_decode is ever attempted, so the bound holds deterministically
regardless of interpreter recursion behavior. The residual
`except RecursionError` clause stays as defense-in-depth for whatever lies
within the bound.

Restores the excessive-nesting regression to a real deep payload (not a
monkeypatch) now that this bound makes the real case reproducible
everywhere; keeps the synthetic RecursionError-from-the-decoder test as
supplemental coverage per review request. Adds a within-bound acceptance
test and an escaped-quote-inside-a-string test (the latter exercises the
scanner's escape handling, which a real deep-nesting payload alone does
not reach).

Also corrects the PR description's stale claim that the RuntimeError
message embeds a scrubbed, length-bounded copy of the raw model response
(the MAX_LLM_RESPONSE_LOG_CHARS constant it named no longer exists);
current source logs only a length and SHA-256 fingerprint, never raw
content -- flagged in the same review comment.

Full suite: 2182 passed, 1 skipped, 21 subtests. 100% statement/branch
coverage, 100% docstrings.

* fix(ci): track array nesting in Noema JSON candidate discovery

Review follow-up (seonghobae, PR #1507, current-head "fail-open blocker"):
extract_json_object's top-level-candidate discovery pass (added in
fb1b118 "reject nested Noema JSON recovery") tracked nesting depth via
{/} only, not [/]. So a malformed outer *array* wrapper containing a
complete, valid inner object -- e.g. '[{"decision":"comment",...}' with
a missing closing ] -- let the inner { be seen at depth zero and
wrongly treated as a fresh top-level candidate, "recovering" a verdict
out of genuinely malformed JSON. This is the same class of bug
fb1b118 fixed for a malformed outer *object* wrapper, just not covering
arrays. depth now increments/decrements across both {/} and [/] so a {
is a candidate only when truly unwrapped by any container.

Also fixes a test-quality bug CodeRabbit flagged in the same review
round: test_noema_superseded_cleanup_selects_only_other_heads_of_same_pr
passed the jq selector's $current as a string via --arg, but the
production invocation (.github/workflows/noema-review.yml) passes it as
a number via --argjson. jq ranks every number below every string, so
the selector's directional `.id < $current` guard was vacuously true
for every fixture row regardless of actual id values -- the test's
assertion held only because the other (name/PR/head) guards still
narrowed correctly, not because the directional guard was exercised.
Restoring the correct numeric type surfaced that the fixture's ids were
also unrealistic (the "current" run had a lower id than the "old"
sibling it should supersede, backward from GitHub's monotonically
increasing run ids); corrected the fixture so "current" has the
highest id, matching real semantics and this repo's sibling
bash-executed test (test_superseded_cleanup_preserves_current_and_newer_run_ids)
that already covers the directional guard correctly.

Full suite: 2184 passed, 1 skipped, 21 subtests. 100% statement/branch
coverage, 100% docstrings.

* fix(ci): wake OpenCode gate without runner polling

* docs: record deterministic Noema depth bound

* test(ci): disambiguate required workflow wake

* fix: bind review continuations to exact PR head

* fix: bound required review receipt lookup

* fix(ci): bound required-run receipt lookup

Signed-off-by: Seongho Bae <[email protected]>

* docs: align bounded receipt lookup window

* fix(ci): bind review wake to required run

Signed-off-by: Seongho Bae <[email protected]>

* fix(ci): validate the referenced wake run on head_sha, not display_title

Devin Review on #1507 flagged the "Wake exact-head required OpenCode
workflow" step's selector as unable to match any run, reasoning that
pull_request_target's reported head_sha is the trusted base revision
rather than the PR head. Verified directly against this org's live
GitHub API data (both ContextualWisdomLab/.github's own PRs and a
sibling repo, noema, consuming the workflow via the org required-
workflow ruleset): head_sha is in fact the PR's actual head commit in
both cases, not the base -- that part of the finding's premise does
not hold, and scripts/ci's own collect_current_head_strix_workflow_runs
already relies on this same, correct, working head_sha semantics.

A concurrent session's prior commit on this branch (bind review wake
to required run) already fixed how the run is *found* -- threading the
triggering run's own $GITHUB_RUN_ID through the repository_dispatch
payload as required_run_id instead of searching by field -- but its
post-fetch *validation* of that run kept the same broken checks the
original selector used: `workflow_url | contains("/actions/
required_workflows/")` and `display_title == "Required OpenCode Review
{repo}#{pr}@{sha}"`. Verified empirically (live REST API queries against
both contexts): `name`/`display_title` only carry that rendered
run-name when opencode-review.yml fires as a native pull_request_target
trigger on its own defining repo; on a sibling repo consuming it
through the required-workflow ruleset -- this repo's actual central-hub
use case -- both fields collapse to the bare workflow name / plain PR
title with no PR or head embedded, while `workflow_url` only contains
"/actions/required_workflows/" in that same ruleset case. No real run
ever satisfies both checks at once, so validation always rejected the
correctly-referenced run regardless of context.

Replace the validation with head_sha exact-match (mirroring the Strix
helper's proven pattern) alongside the existing id/event/path checks.
Updates the contract test's pinned assertions and adds direct jq-level
regression coverage (mirroring this file's existing runtime_verdict
pattern) proving: the referenced run is matched using only id/event/
path/head_sha, with no reliance on name or display_title; a referenced
run whose head_sha has since moved on (Devin Review's "another PR or
head" concern, now reinterpreted for an id-based reference: a
superseded run or a stale/forged required_run_id) is rejected; and a
referenced run for a different required workflow (Strix) is rejected.
The existing end-to-end fake-GitHub script test is updated to a
realistic ruleset-shaped fixture (no PR/head in name or display_title)
proving the real success path doesn't depend on either field. Also
re-pins REVIEW_DISPATCH_BLOB_SHA to match the edited dispatch workflow.

Full suite: 2190 passed, 1 skipped, 21 subtests. coverage report: 100%
on scripts/ci/. interrogate: 100% docstrings.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix: preserve immutable required review identity

* fix(ci): make Noema JSON nesting a bracket-type-aware stack, not a counter

Devin review on PR #1507's current head: extract_json_object's candidate
discovery and _json_nesting_within_bound both tracked nesting via a plain
depth counter that incremented on any of {/[ and decremented on any of
}/], regardless of matching type. A mismatched closer -- a stray ] where
the enclosing container is a {, or vice versa -- decremented the shared
counter anyway, so it could prematurely signal "the outer wrapper is
closed" while genuinely deeper or still-open structure followed.
Reproduced directly: '{"broken": ]{"decision":"comment","summary":"ok",
"findings":[]}' (a } expected after "broken": but a ] appears instead)
made the naive counter return to zero at that ], so the inner recovery
object's own { was wrongly treated as a fresh top-level candidate and
"recovered" out of genuinely malformed JSON -- the same fail-open bug
59eda8b closed for array wrappers, reopened via bracket-type confusion.

Both functions now use a bracket-type stack (push "{"/"[' on open, pop
only on a matching close; a mismatched closer is a no-op, never popping).
Removed a related dead branch this exposed: _json_nesting_within_bound is
only ever called with text[start] == "{" (its own documented contract),
so the stack's bottom element is always "{" and can never be emptied by
a "]" -- only "}" can legitimately signal completion.

Full suite: 2186 passed, 1 skipped, 21 subtests. 100% statement/branch
coverage, 100% docstrings.

* fix(ci): abort Noema JSON candidate discovery on any structural mismatch

Devin review follow-up on 7df533e: making a mismatched closer a stack
no-op (ignored, not popped) closes the specific case Devin first
reported, but not the general one. A LATER, otherwise-well-formed
bracket pair can still legitimately re-close the stack down to empty
despite an earlier mismatch, so a subsequent { would again look like a
fresh top-level candidate. Reproduced: '[} ] {"decision":"comment",...}'
-- the stray } is correctly a no-op against the open [, but the
following ] still validly closes that [ (matching type), and the {
after it was then wrongly treated as a fresh top-level candidate and
"recovered" out of genuinely malformed JSON.

Both } and ] handlers now break out of candidate discovery entirely the
moment they see a closer that cannot legally match the innermost open
bracket (nothing open, or the innermost open bracket is the other
type), rather than merely no-opping and continuing to scan. Any
closer this malformed anywhere in the response is now treated as proof
the whole response cannot be trusted to contain a clean top-level
object from that point on, not just proof that one bracket group
failed to close.

Full suite: 2188 passed, 1 skipped, 21 subtests. 100% statement/branch
coverage, 100% docstrings.

* fix: wake required review after scheduler retry

* test: cover missing review run URL

* fix(ci): complete required-run selection

Signed-off-by: Seongho Bae <[email protected]>

* test: cover status context pagination guards

* fix(ci): fix sibling Noema cleanup evasion and scheduler wake gaps

Devin Review findings on PR #1507:

- "Sibling Noema runs evade cancellation": noema-review.yml's close and
  live-head-supersession cleanup jobs matched runs only by an exact
  `.name ==` filter and a display_title prefix carrying this workflow's
  rendered run-name. GitHub does not consistently render that run-name for
  an organization-required-workflow pull_request_target run materialized
  in a sibling repository (confirmed live against real contextual-orchestrator
  and noema runs during this fix) -- both fields can collapse to the bare
  workflow name and the plain PR title there, so neither cleanup sweep ever
  matched a sibling PR's runs. Both selectors now additionally match via
  GitHub's own pull_requests[] array (reliably populated here because this
  job only processes same-repository, non-fork PRs) and pin workflow
  identity via the run object's own `.path` instead of `.name`. The
  live-head exclusion in the supersession step is reinforced with a direct
  `.head_sha` comparison alongside the existing display_title-based one.

- "Older review run remains blocking": matching_actions_run_id selected the
  first predicate match scanning the rollup in reverse, which is only the
  newest match when GitHub happens to return contexts chronologically --
  not guaranteed. Now ranks every match with the same check_run_recency_key
  signal used elsewhere in this file to resolve reruns.

- "Large check rollups never wake": the GraphQL rollup fragment caps at 100
  contexts, so a PR with more already-accumulated checks can push the real
  Required OpenCode Review run past that page. dispatch_opencode_review now
  falls back to discover_opencode_required_run_id, a bounded REST lookup
  scoped server-side to the exact event, workflow path, and head SHA.

Full validation: coverage run -m pytest tests -q -> 2198 passed, 1 skipped,
21 subtests; coverage report -> 100% statement/branch on scripts/ci;
interrogate -> 100% docstring coverage.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* test: exercise repeated status context pages

* test(ci): exercise status pagination guard

Signed-off-by: Seongho Bae <[email protected]>

* test: refresh trusted dispatch blob pin

* test(opencode): align head-advance contract

Signed-off-by: Seongho Bae <[email protected]>

* test: align head-advance dispatch contract

* fix: authorize required review wake

* fix: preserve live Noema review after cancelled trigger

* fix(scheduler): treat sole head-adopting repository_dispatch run as current

Devin Review finding on PR #1507 ("Live-head reviews retain stale identity"):
a run dispatched for a supplied head can have its validate-pr-metadata step
adopt a live head that advanced after dispatch (the #1533 warn-and-proceed
path) and review it end-to-end, while the run's immutable run-name/
display_title still renders the stale supplied head. active_review_run_refs
was comparing that stale title against the live PR head to decide current
vs. stale, so a scheduler pass could misclassify and force-cancel a review
that was correctly reviewing the live head, then dispatch duplicate work.

Fix: only fall back to the per-title-head comparison when two or more
repository_dispatch runs match the same target-repo/PR-number title prefix
(a genuine overlap -- most plausibly the workflow's own cancel-in-progress
concurrency group not having finished cancelling an actually-superseded run
yet). A sole match is always current, since that same concurrency group
guarantees there is no other run to prefer over it.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw

* fix: isolate cancelled Noema notifications

* docs: clarify live-bound review ownership

* Revert "fix(scheduler): treat sole head-adopting repository_dispatch run as current"

This reverts commit b1232df.

* docs: restore exact-head dispatch contract

* test: restore exact-head dispatch contract

* fix: reject malformed Noema preface

---------

Signed-off-by: Seongho Bae <[email protected]>
Co-authored-by: Claude <[email protected]>
Co-authored-by: seonghobae <[email protected]>

… case

The merge of origin/main brought in inspect_and_review()'s new required
expected_head parameter (#1507's stale-head repair-retry work). The
legacy-review regression case added in this branch's own prior commit
predated that signature change locally and was missed during the merge's
auto-resolution of noema_review_gate.py, leaving it as the one call site in
this test still using the old two-argument form.

…st (#1877)

* fix(tests): repair changed-scope drift and stale noema cancel-step test

Two pre-existing tests/ failures blocked the unscoped pytest discovery
run in agent-review-runtime-quality-ci.yml's review-repair contracts
step, unrelated to the hourly-cron fixes in #1875:

- strix.yml's changed-scope job if: had drifted onto a multi-line `>-`
  block scalar when PR #1869 added converted_to_draft handling, and the
  extra continuation lines broke byte-identical parity with
  security-scan.yml/sast-semgrep.yml's copies. Collapsed back to one
  physical if: line with the same expression.
- test_noema_close_cleanup_selects_only_the_closed_pr_across_shared_display_titles
  still targeted the pre-#1869 step name/env vars
  (CLOSED_PR_NUMBER) that PR #1869 renamed to "...for the inactive pull
  request" / INACTIVE_PR_NUMBER/INACTIVE_PR_HEAD_SHA/PR_ACTION when it
  generalized noema-review.yml's cleanup to also cover
  converted_to_draft and added a live_target_matches re-verification.
  tests/test_noema_review_gate.py's equivalent tests were already
  updated; this one was missed. Updated the step name/env vars and
  taught the fake gh to answer the new live-PR lookup -- the PR #1507
  pull_requests[] cancellation-scoping invariant it protects is
  unchanged and still correctly implemented in production.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX

* fix(tests): mock fetch_pr in Strix rerun job selection test

test_dispatch_strix_reruns_scan_job_not_sibling_publisher only mocked
rerun_actions_job, leaving dispatch_strix_evidence's live_dispatch_head_matches
call to invoke the real fetch_pr. In any environment with a real gh CLI on
PATH this hits the actual GitHub API for a synthetic PR that does not exist
there, returning a live/head mismatch ("stale_head") instead of the expected
"rerun"; without gh at all it fails even earlier with a missing executable.

Add monkeypatch.setattr(sched, "fetch_pr", lambda *_args: [pr]) so the
live-head re-read observes the same fixture pr as authoritative, consistent
with how every other GitHub call in this test path is already isolated.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX

* fix(tests): align Strix admission assertion

* fix(tests): align daily review recovery fixtures

---------

Co-authored-by: Claude <[email protected]>