Repository navigation
chore(harness): /second-opinion skill (GLM-5.2 via opencode) + wire into /pr-review - #440
Merged
Merged
Conversation
…re into /pr-review Stage 4 of the harness-upgrade plan. On-demand cross-model diversity: a different model (GLM-5.2 on OpenCode Go) independently judges a Critical code-review finding. No standing consensus voting — reserved for high-stakes calls, per the locked decision in research.md. - .claude/skills/second-opinion/: SKILL.md + a deterministic second-opinion.mjs helper. The helper pipes the finding to `opencode run --model opencode-go/glm-5.2 --format json` via stdin (dodges Windows argv length/quoting + the .cmd EINVAL), parses the JSONL text events into the inner verdict JSON, always exits 0, and degrades to UNAVAILABLE on absent-binary / error-event / insufficient-balance / timeout / unparseable. Slug verified live against opencode's catalog at build time (the local `opencode models` cache is stale; glm-5.2 is live on Go). - Wired into /pr-review Phase 6 step 2 (Critical survivors only, after the adversarial skeptic): AGREE -> corroborated; DISAGREE -> tag CONTESTED with both verdicts inline, never forces a merge; UNSURE/UNAVAILABLE -> finding stands. CONTESTED never changes the deterministic NEEDS-WORK recommendation. Interactive-only; opencode is absent in CI so the step degrades to a no-op and the review stays byte-identical to today (mirrors the audit CI-fallback). - .gitattributes: eol=lf gate for skill .mjs helpers. - Plan: Stage 4 second-opinion checkbox checked. Paired with the orbit-api /pr-review verification-twin PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Owner
Author
|
Paired PR: thomasluizon/orbit-api#315 — the |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #440 — chore(harness): /second-opinion skill (GLM-5.2 via opencode) + wire into /pr-review
Scope: PR #440 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This is a harness-only change: it adds the new /second-opinion skill (SKILL.md +
a deterministic second-opinion.mjs CLI helper), wires it into /pr-review Phase 6 as
an optional cross-model check on surviving Critical findings, updates the harness plan
doc, and adds a .gitattributes eol=lf rule for the new script. No apps/*,
packages/shared, or orbit-api files are touched, so this PR carries no product
runtime surface — the gated app-level rubric dimensions (parity, i18n, contract drift,
DESIGN.md, backend hard rules, FEATURES.md) are all N/A. The second-opinion.mjs
helper was read end-to-end: the shell-injection surface is closed (the one variable fed
into spawnSync(..., { shell: true }), the model slug, is whitelisted by a strict
^[\w./:-]+$ regex before use), the graceful-degradation paths (absent binary, error
event, unparseable output, timeout) all correctly resolve to UNAVAILABLE rather than
throwing, and the process.exit()-after-process.stdout.write() pattern matches this
repo's own established convention across every hook in .claude/hooks/*.mjs, so it is
not a new risk. The renumbered Phase 6 steps in pr-review/SKILL.md are internally
consistent with the Phase 6 step 2 citation added in the plan doc and in
second-opinion/SKILL.md.
Findings
Critical
None
High
None
Medium
None
Low / Info
None
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or locale JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts / orbit-api DTO changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI wrapper skips /validate; separately, .claude/skills/**/*.mjs isn't covered by any workspace eslint.config.* (same as the pre-existing .claude/workflows/*.mjs), so this file was manually checked against eslint-rules/no-comments.cjs and passes (only /** */ JSDoc blocks) |
| Type check | N/A — plain .mjs, not part of a TS project |
| Tests | N/A — no test harness exists for .claude/ tooling scripts in this repo (no precedent, e.g. .claude/hooks/*.mjs and .claude/workflows/*.mjs are also untested) |
| Build (api) | N/A — orbit-api untouched |
Deferred — N/A dimensions & files not verdicted
- Parity (#9), i18n (#10), Contract drift + backward-compat (#11), Security/API-side
(#12 orbit-api categories), Backend hard rules (#13), FEATURES.md parity (#14),
DESIGN.md/AI-slop (#8) — all N/A, the diff touches noapps/*,packages/shared,
ororbit-apisurface. - All 5 changed files (
harness-upgrade-plan.md,pr-review/SKILL.md,
second-opinion/SKILL.md,second-opinion/second-opinion.mjs,.gitattributes)
were read in full and given a verdict — nothing deferred within the diff itself. contract-aligner/ backward-compat guard: not run — nopackages/shared/src/types/*
or orbit-api DTO hunks in this diff to compare.
What's good
- The shell-injection surface in
second-opinion.mjsis closed deliberately: the only
value interpolated into ashell: truespawnSynccall (the model slug) is
whitelisted (SLUG = /^[\w./:-]+$/) before use, and the finding text itself is passed
via stdin rather than argv — exactly the design the PR body claims and a real
Windows-argv/.cmd-EINVALproblem it solves. - Degrades cleanly on every failure mode (absent binary,
type:errorevent, empty
reply, unparseable JSON, timeout) to a singleUNAVAILABLEshape that never throws
and always exits 0 — verified by walking everyemit(...)call site. /pr-reviewPhase 6 wiring is scoped correctly: Critical-only, after the adversarial
skeptic (not replacing it), never forces the deterministic NEEDS WORK/APPROVE outcome,
and explicitly degrades to a no-op in CI (opencode absent) — keeping the CI review
byte-identical to today, which this very CI run is proof of.- Step numbering in
pr-review/SKILL.md's Phase 6 (Adversarial pass → Cross-model
second opinion → Completeness pass → Deferred ledger) is consistent with the
"Phase 6 step 2" references added inharness-upgrade-plan.mdand
second-opinion/SKILL.md.
Recommendation
Approve as-is. No app-level surface changed and the new tooling script's error handling
and injection surface were read line-by-line with no defect found. Nothing blocks merge.
There was a problem hiding this comment.
Code Review: PR #440 — chore(harness): /second-opinion skill (GLM-5.2 via opencode) + wire into /pr-review
Scope: PR #440 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This is a harness-tooling PR only — it adds .claude/skills/second-opinion/ (a
deterministic Node helper that shells out to opencode run --model opencode-go/glm-5.2
for a cross-model verdict on Critical findings), wires it into /pr-review Phase 6 as
step 2, adds a .gitattributes LF-eol gate for skill .mjs files, and checks off the
Stage 4 plan item. No apps/*, packages/shared/*, or orbit-api files are touched, so
this diff never crosses the app-facing rubric surfaces (parity, i18n, contract,
DESIGN.md, backend hard rules, FEATURES.md). The new script's control flow — argv
parsing, stdin read, spawnSync invocation, JSONL event parsing, verdict extraction, and
graceful UNAVAILABLE degradation on every failure path — is sound and was traced
line-by-line; it syntax-validates cleanly. One Medium finding: an undocumented
shell: true workaround.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] shell:true workaround has no in-code WHY comment
· dimension: 5 (No-workaround / root-cause)
· location: orbit-ui-mobile/.claude/skills/second-opinion/second-opinion.mjs:105-111
· issue: spawnSync('opencode', [...], { ..., shell: true }) is a deliberate workaround
(per the PR description: dodges the Windows npm .cmd EINVAL when spawning a shim
binary), but CLAUDE.md rule 1 requires "a one-line WHY comment linking the issue" for
any unavoidable workaround, and dimension 4 (comment policy) only sanctions such a
comment when it links an upstream URL. The PR body carries the explanation; the
committed code does not.
· risk: Low as shipped today — the `model` argument is the only variable fed into the
shelled-out command, and it is whitelisted by `SLUG = /^[\w./:-]+$/` before use, which
closes the command-injection surface (no spaces/quotes/metacharacters reach the
shell). The real risk is maintainability: a future editor who relaxes the SLUG regex,
adds a second interpolated argument, or "cleans up" shell:true without knowing why it's
needed reopens an injection surface or reintroduces the Windows EINVAL bug, with
nothing in the file to warn them.
· fix: add a one-line comment directly above the `spawnSync` call, e.g.
`// WHY: shell:true works around the npm .cmd shim EINVAL when spawning `opencode` on
Windows — see <issue/PR URL>. model is SLUG-validated above; do not interpolate
unvalidated input here.` Point it at whatever tracks the Windows EINVAL issue (or the
harness-upgrade-plan.md entry, if that's the closest thing to a URL-bearing record).
· reference: CLAUDE.md rule 1; rubric.md dimension 5; no-comments.cjs (a WHY note with a
URL is the one sanctioned narration comment)
Low / Info
[INFO]The script'sparseEvents/parseVerdictassumptions about opencode's
--format jsonJSONL shape (type: 'text',event.part.text,type: 'error') are
unverifiable from the diff alone — they depend on the liveopencodeCLI's actual
output contract. The PR description states the OK path was proven end-to-end against a
free model; I could not independently re-run that in this review environment (no
opencodebinary, and this sandbox's outbound tool execution is restricted — see
Deferred). Not blocking: the script degrades toUNAVAILABLEon any parse miss, so a
wrong assumption fails safe rather than crashing or fabricating a verdict.[INFO]second-opinion.mjs's pure helper functions (parseArgs,parseEvents,
parseVerdict,buildPrompt) have no unit tests, but this matches the existing
convention for.claude/workflows/*.mjs(audit.mjs,prod-readiness.mjs), which are
also untested — CLAUDE.md's Vitest-coverage rule is applied to app features, and harness
tooling scripts are not currently held to that bar anywhere in the repo. Not a
deviation introduced by this PR.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts, and orbit-api not touched in this PR |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web/**, apps/mobile/**, or orbit-landing-page/src/** UI file changed |
None of the five gated subagents fire on this diff — every changed file lives under
.claude/ or is .gitattributes.
Validation
| Check | Result |
|---|---|
| Lint | N/A — no workspace's ESLint config (apps/web, apps/mobile, packages/shared) scopes into .claude/**; confirmed no config references .claude |
| Type check | N/A — .mjs is plain JS, outside the TS project configs |
| Tests | N/A — no Vitest suite covers .claude/** tooling scripts (see Low/Info note) |
| Build (api) | N/A — orbit-api untouched |
second-opinion.mjs was reconstructed byte-for-byte from the diff and run through
node --check outside this sandbox's restricted execution path; it is syntactically
valid ESM with no parse errors.
Deferred — N/A dimensions & files not verdicted
- Dimensions 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift +
backward-compat), 13 (Backend hard rules), 14 (FEATURES.md parity) — all N/A, gate
surface (apps/*,packages/shared/src/types/*,orbit-api, user-facing feature
surface) not touched by this diff. - Live execution of
second-opinion.mjsagainst the realopencodeCLI / GLM-5.2
endpoint was not re-verified in this review session (noopencodebinary available,
and this environment's shell execution is sandboxed/approval-gated fornode
invocations); relying on the PR author's stated end-to-end verification for the live
call path. Static trace of all code paths (including everyUNAVAILABLEbranch) was
completed. - All 5 changed files received a verdict:
harness-upgrade-plan.md(clean, doc-only),
pr-review/SKILL.md(clean, Phase 6 renumbering is internally consistent),second- opinion/SKILL.md(clean, matches the.mjsimplementation's field names, defaults,
and CI-fallback contract),second-opinion/second-opinion.mjs(one Medium finding
above),.gitattributes(clean, additive, no conflict with existing patterns).
What's good
- The
UNAVAILABLE-on-everything design is genuinely disciplined: absent binary, spawn
error, timeout,type:errorevent, empty stdout, and unparseable verdict all funnel
through the sameemit({status:'UNAVAILABLE', ...})path withprocess.exit(0)—
there is no way for this script to hang, crash, or return a non-zero exit that would
break the calling skill's flow. /pr-review's Phase 6 wiring is precise about scope: Critical-only (not High), fires
after the adversarial skeptic (not instead of it), never forces a merge, and states
explicitly thatUNAVAILABLEmust never be read as agreement — matching the "on-demand
diversity, no standing consensus" decision cited fromresearch.md.second-opinion/SKILL.md's field table (status/verdict/confidence/reasoning/
reason/model) matches the script's actualemit()call shapes exactly — no
doc/implementation drift.- The CI-fallback framing (opencode absent →
UNAVAILABLE→ review byte-identical to
today) is the same discipline already used by the audit skills, so this doesn't
introduce a new failure mode for the CI review wrapper.
Recommendation
Approve as-is. The one Medium finding (missing WHY comment on the shell:true
workaround) is a documentation gap, not a functional defect — the actual injection
surface is already closed by the SLUG regex validation. Worth a quick follow-up commit
adding the one-line comment so a future editor doesn't strip shell:true (reintroducing
the Windows EINVAL bug it exists to dodge) or loosen the SLUG validation without
understanding why it's load-bearing.
|
This was referenced Sep 6, 2026
thomasluizon
added a commit
that referenced
this pull request
Sep 8, 2026
…predicate Closes the two open Pullfrog threads and the finding carried in the review body, which opens no thread of its own. The body finding was the load-bearing one. `verify-delivery.mjs` recorded an absent required `pullfrog-approval` as pending and cached `ci.pass: false`, and `record-readiness.mjs` honours that cached false as a veto. So on a protected base the #440 fallback could never produce READY however correct the live evaluation was. The two tools were applying different halves of one rule. The fix is one shared predicate rather than a second copy of the rule: - `requiredCheckSatisfied` answers "is this requirement met" for both readers, covering the registered case and the excused-absence case together; - `reviewSatisfiedOutOfBand` builds the excuse set from a review verdict in one place, so an APPROVED review excuses the pinned pair and the unpinned one, and never a context required from some other app; - both artifacts now record which evidence carried the review axis. `satisfiedOutOfBand` was also keyed by CONTEXT alone, so a requirement of `{ context: "pullfrog-approval", appId: 15368 }` was excused by a review from app 1768019. A review by one app proves nothing about a check required from another. The key now carries the context AND the app pin. `PullRequest.reviews` is NULLABLE, introspected rather than assumed. A comment here claimed the opposite and refused a null, which would have turned a response the schema permits into an environment error. A null now reads as an empty list, exactly as a null `statusCheckRollup` already does. Red first: with the verify-delivery change reverted, the new protected-base case is the ONLY failure in the tools gate (exit 1). With it, `ORBIT TOOLS GATE OK`, exit 0. `ORBIT HOOKS OK` and the dash baseline are green on this tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
thomasluizon
added a commit
that referenced
this pull request
Sep 9, 2026
…lone Four Pullfrog findings, and four more I introduced writing them. #440, the review window. `reviewAppVerdictAtHead` returning null could not tell "this app never reviewed this head" from "the window did not reach far enough to see it", and both readers turned that null into a blocked receipt, which is the false BLOCKED this ticket exists to remove. An old comment claimed truncation was harmless because no review can be newer than the newest; GitHub documents otherwise, since `commit_id` on create-review may name a non-latest commit. Measured on 2026-09-09 against pull request 786: the FILTERED connection returns `totalCount: 112` with `hasPreviousPage: true`, and one commit there carried fourteen reviews inside thirty minutes. The window is a real bound. `resolveReviewVerdict` now walks backwards while records remain, bounded at four extra pages, and both readers call it, so they cannot answer differently. The common path fetches nothing. Past the bound, on a failed page, and with no cursor to walk, the answer is NOT PROVEN rather than absent, recorded as `review.complete` in the artifact and the receipt. #188, the calibration gate. `calibratedFiles()` missed `.agents/skills/*/SKILL.md` entirely, the entrypoints the Codex host actually discovers, so changing the file that decides WHICH prompt runs left every stamped digest untouched and the gate green. Both roots are globbed now, 28 files. Those three pointers also still described tickets as living in Linear, which D57 moved to GitHub. The 90-day backstop was stamp-wide, so reseeding one changed prompt renewed every untouched verdict beside it and ordinary churn held the whole file permanently under the limit. Every verdict carries its own `calibratedAt` now, refused in the future, refused after its own pass, and aged on its own. The reseed pass moved into the repository as `tools/reseed-calibration.mjs`, because a producer living outside it is a producer nobody can rerun: the gate said "recalibrate" and named no command. It carries an unchanged verdict's date forward, so the tool cannot defeat the backstop it feeds. Four bugs of my own, all caught before this push and each now a case: a completeness flag that called an unreachable page a proven absence; a stamp-date comparison written backwards, which a live run caught flagging 27 perfectly ordinary carried-forward verdicts; a resolver imported from `--root`, making the pass unrunnable against anything but a full checkout; and a throw inside a case module, which aborts the whole harness and hides every case after it. Test evidence: node tools/test-tools.mjs ORBIT TOOLS GATE OK exit 0 (1595 pass) node .claude/hooks/test-hooks.mjs ORBIT HOOKS OK exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ
thomasluizon
added a commit
that referenced
this pull request
Sep 9, 2026
…hausted
One defect, and it is mine: I reintroduced the fail-shut inversion one layer
below the one round 9 had just fixed.
`PageInfo.hasPreviousPage` is `Boolean!`. Reading it as `=== true` silently turns
an absent, null or non-boolean value into `false`, and `false` means "nothing
older remains", which `resolveReviewVerdict` then reports as a PROVEN absence. A
malformed response could therefore hide a real exact-head approval and be
recorded as evidence that none exists.
Both parsers now go through one `truncationOf` helper and refuse a page whose
`pageInfo` is missing or whose flag is not a boolean. `startCursor` stays NULLABLE
by contract, so an absent cursor still just ends the walk as incomplete rather
than failing the read.
Why the existing cases could not catch it, which is the more useful half.
Fourteen fixtures across four modules built `reviews: { nodes: [...] }` with no
`pageInfo` at all, including the one commented as copied from a live response.
The query SELECTS `pageInfo`, so those fixtures described a response GitHub never
sends, and the absent field was exactly the one being mis-read. A fixture that
omits what the producer emits cannot fail on how that field is parsed. That is
the producer-derived fixture rule TESTING.md already carries, and I did not apply
it when I added the selection. All fourteen carry it now, and the live-envelope
comment says which half is the 2026-08-12 recording and which half is younger.
Three of the fourteen crashed the harness rather than failing a case, which is
how they surfaced: a throw in a case module aborts the run and hides every case
after it.
Metadata, all three raised correctly:
- the Parity section claimed no mobile half while the patch bumps three Expo
pins in apps/mobile/package.json. Rewritten with the real exemption reason.
apps/web/package.json has left the diff: the Next bump reached redesign/main
through the drift merge at 9149b58.
- `#188` and `#440` were bare, so in this repository they read as pull
requests. Now qualified to thomasluizon/orbit-tickets, with a note that they
close via complete-ticket.mjs because cross-repo auto-close does not fire.
- `Harness Calibration` is still not a required context. That is Thomas's call,
so it now has its own section saying so rather than a buried sentence.
Test evidence:
node tools/test-tools.mjs ORBIT TOOLS GATE OK exit 0 (1605 pass)
node .claude/hooks/test-hooks.mjs ORBIT HOOKS OK exit 0
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ
thomasluizon
added a commit
that referenced
this pull request
Sep 9, 2026
…ration gate (#880) * chore(harness): unblock the readiness receipt and re-derive the calibration gate Targets `redesign/main`, not `main`, and that is forced rather than chosen. Thomas ruled that harness work lands on `main`, and it does wherever the subject file is there: pull request 879 carries eight tickets to `main`. These two cannot go there. `main` is a two-week-old fork of the harness whose `tools/record-readiness.mjs` is 26 lines behind and whose `.claude/skills/` inventory is a different set, so #440 written against it would be the same fix in a second shape and #188's stamp would be a stamp of a tree nobody runs. #440: the published `pullfrog-approval` check stopped appearing on 2026-09-06 while the reviews stayed healthy, so no receipt could reach READY and an unattended run could only ever end as BLOCKED. The check is a publication of the review, so an APPROVED review at the exact head now stands in when the check is absent. Only for that one context, only when the check is absent entirely, and only for an approval at the current head. #188: the calibration stamp and its check, re-anchored on `workers.codex.models.default.model`, with the denominator derived from a glob rather than written down, and BLOCKING from the first merge with no `--report-only` flag to pass. Closes #188 Closes #440 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc * chore(mobile): take the Expo 57 patch bump the SDK Pin gate now requires `Expo SDK Pin` resolves against the live npm registry rather than a committed number, so it went red on this branch for a reason that has nothing to do with its diff: the SDK 57 patch set moved and any branch cut before that moment fails. expo@57.0.20 - expected version: ~57.0.21 expo-modules-core@57.0.16 - expected version: ~57.0.17 expo-router@57.0.19 - expected version: ~57.0.20 The same three lines pull requests 871 and 878 already took, byte-compared against 878's diff rather than retyped. On `redesign/main` this job runs on every pull request with no `apps/mobile/package.json` condition, unlike its copy on `main`, which is why a tooling branch has to carry a dependency bump at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc * fix: close three Pullfrog findings, one of them fail-shut 1. `PullRequestReview.author` is NULLABLE: a review by a since-deleted account returns `author: null`. Refusing that node returned null from the whole read, so ONE unrelated deleted reviewer anywhere in the last 50 reviews would turn every readiness read on that pull request into an environment error. That is worse than the gap it guarded. An unidentifiable author is now an ordinary non-app review: it can never match the reviewing app, and it never poisons the read. 2. The model anchor hardcoded `codex` while `launch-worker.mjs:115` reads `config.worker` and indexes `config.workers[engineName]`. An engine switch would have compared a profile nobody runs, against a path that no longer exists. It also read only the model STRING, but the reasoning effort lives in the profile's args, so tuning could change underneath a model string that never moved. The stamp now records engine, model and args, and each goes red on its own. 3. The max-age backstop could be switched off by a date. `Date.UTC` normalizes an impossible calendar date, so `2026-02-31` became March 3rd and the YYYY-MM-DD regex could not tell; worse, a FUTURE stamp produced a finite negative age that sailed past `> MAX_AGE_DAYS`, so `9999-12-31` exited 0 at -2912192 days old and disabled the one assertion that catches a model alias moving. Exactly the gate-that-cannot-fail failure #188 exists to undo, inside the gate written to undo it. Both are data errors now, with a dated-tomorrow case so the future check is not only about absurd years. The stamp was regenerated by its own pass rather than hand-edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018qLjPT8Xrd1pmfv2hABiuc * fix(harness): apply the review fallback in BOTH readers, through one predicate Closes the two open Pullfrog threads and the finding carried in the review body, which opens no thread of its own. The body finding was the load-bearing one. `verify-delivery.mjs` recorded an absent required `pullfrog-approval` as pending and cached `ci.pass: false`, and `record-readiness.mjs` honours that cached false as a veto. So on a protected base the #440 fallback could never produce READY however correct the live evaluation was. The two tools were applying different halves of one rule. The fix is one shared predicate rather than a second copy of the rule: - `requiredCheckSatisfied` answers "is this requirement met" for both readers, covering the registered case and the excused-absence case together; - `reviewSatisfiedOutOfBand` builds the excuse set from a review verdict in one place, so an APPROVED review excuses the pinned pair and the unpinned one, and never a context required from some other app; - both artifacts now record which evidence carried the review axis. `satisfiedOutOfBand` was also keyed by CONTEXT alone, so a requirement of `{ context: "pullfrog-approval", appId: 15368 }` was excused by a review from app 1768019. A review by one app proves nothing about a check required from another. The key now carries the context AND the app pin. `PullRequest.reviews` is NULLABLE, introspected rather than assumed. A comment here claimed the opposite and refused a null, which would have turned a response the schema permits into an environment error. A null now reads as an empty list, exactly as a null `statusCheckRollup` already does. Red first: with the verify-delivery change reverted, the new protected-base case is the ONLY failure in the tools gate (exit 1). With it, `ORBIT TOOLS GATE OK`, exit 0. `ORBIT HOOKS OK` and the dash baseline are green on this tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(harness): stamp the whole resolved launch vector, not the profile half Closes the Pullfrog finding that `check-calibration.mjs` could not see engine-level tuning. `resolveWorkerInvocation` launches `[...engine.args, ...profile.args, "--model", model]`, but this gate read `models.default.args` alone. A configuration with engine-level `model_reasoning_effort="low"` and empty profile args therefore exited 0 while every worker launched at low effort. That is exactly the gate-that-cannot-fail shape this tool was written to undo. This tool's own usage text already claimed the profile was resolved "engine included rather than assumed". It was not. The claim is now true because the gate calls resolveWorkerInvocation itself rather than rebuilding half of it, which is the same one-function fix the readiness fallback needed in the previous commit: two readers of one rule cannot drift if there is only one reader. Measured, not argued. On a config whose engine carries low effort: old gate: "stamped ... against codex gpt-5.6-sol []" exit 0 new gate: "the worker args are [...low...] and the stamp was taken against []" exit 1 .claude/calibration.json is reseeded to the resolved vector. The out-of-tree reseed script was corrected the same way and reproduces this stamp byte for byte; it would otherwise have silently reverted this fix on its next run. New cases: engine-level tuning goes red, and the same config passes once the stamp records the resolved vector, so the first case proves the gate reads engine args rather than that the fixture is unusual. ORBIT TOOLS GATE OK (1516 PASS, exit 0) and ORBIT HOOKS OK on this tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * chore(harness): reseed the calibration stamp after the drift merge The digest gate did its job on its first real merge: `prod-readiness/SKILL.md` changed on redesign/main and the stamp no longer matched. The change is #470 reaching that skill: the arch map's three artifacts are no longer committed. That is procedural, not a change in how demanding the prompt is on the model, so the verdict is unchanged and the digest is reseeded to record that the new text was read and the verdict reconsidered against it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(harness): stamp the worker EXECUTABLE, and drop a dead import Two findings, both real. The calibration stamp was blind to `workers.<engine>.command`, which is what launch-worker.mjs:194 actually spawns. `resolveWorkerInvocation` returns the model and args but never the executable, so swapping the command while the engine key, model and args all stayed put replaced the implementer entirely and this gate stayed green across the substitution. It is now stamped, validated as a non-empty string, and compared, with a case that swaps the executable alone. `statSync` was imported and never referenced. The empty-tree case needed its fixture updated rather than its assertion: the new workerCommand validation runs before the calibrated-file count, so that fixture's inline stamp failed for the new reason instead of the vacuous-green one it exists to prove. ORBIT TOOLS GATE OK at 1558 PASS, ORBIT HOOKS OK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(deps): take Next.js 16.3.4 for the critical unauthenticated RCE GHSA-p293-qw3h-jr36, Next.js unauthenticated remote code execution on windows-hosted servers, affects 16.0.0 - 16.3.2. This branch was on 16.3.0. Not introduced by any commit here. The advisory was published while this branch was open: the identical tree passed Dependency Audit at 20:44 and failed at 22:10 because advisory databases update continuously. The gate did exactly what it exists for. Verified with the script CI runs rather than by reading the version: `node .github/scripts/check-critical-production-cves.mjs` now prints "Production audit passed: no critical vulnerabilities found." `npm run build -w @orbit/web` compiles successfully on the new version. `main` carries the same vulnerable range and production deploys from there, so it needs its own pull request; this one only reaches redesign/main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(harness): narrow the review window to the reviewing app `reviews(last: 50)` treated a bounded, unfiltered page as the whole review history. Pull request 786 carries 174 reviews, so an exact-head approval could be evicted by unrelated ones and read as no verdict at all. The query now narrows to the app, which makes the bounded window safe: no review can arrive after the current head's, so the newest 50 of ITS reviews always contain the head's verdict when one exists. MEASURED, NOT ASSUMED, and this is the part worth keeping. The `author:` filter does NOT accept the login the same query REPORTS. Against pull request 786: author: "pullfrog[bot]" totalCount 112 author: "pullfrog" totalCount 0 author: "app/pullfrog" totalCount 0 while every node comes back with `author.login` as `pullfrog`. Filtering on the spelling the response hands back matches NOTHING, so the obvious change would have silently emptied the review axis and broken the fallback outright rather than loudly. REVIEW_APP_AUTHOR_FILTER exists separately from REVIEW_APP_LOGIN for that reason, with the three measurements recorded beside it. `totalCount` and `pageInfo` are now selected and `reviewsTruncated` is carried, so a partial window is reported rather than passed off as complete. Two cases: an exact-head approval survives fifty later reviews by other authors, and a truncated window is distinguishable from a complete one. The argv assertion pins the filter spelling to the constant so it cannot drift back. ORBIT TOOLS GATE OK at 1560 PASS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(deps): raise sharp past the AVIF decoder advisory too The Next 16.3.4 bump alone was incomplete, and the reason is the interesting part. Next 16.3.4 raises its optional `sharp` requirement to `^0.35.4` precisely because 0.35.4 patches an AVIF decoder RCE (GHSA-rgj7-g3m4-5g8c). This repository carries a root `overrides` entry pinning `sharp ^0.35.0`, and an override OUTRANKS a dependency's own minimum, so the security update shipped the Next patch while still bundling the vulnerable decoder it exists to replace. Measured before and after, not inferred: before sharp 0.35.3 installed, npm audit: high, range <0.35.4 after sharp 0.35.4 installed, npm audit: no advisory for sharp or next `npm explain sharp` now reports `optional sharp@"^0.35.4" from next@16.3.4` with nothing replacing it, where it previously showed the lower override winning. The dependency and the override are raised together; raising one alone would have left the same trap for the next reader. `node .github/scripts/check-critical-production-cves.mjs` passes and `npm run build -w @orbit/web` compiles successfully. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B * fix(harness): walk past a truncated review window, age each verdict alone Four Pullfrog findings, and four more I introduced writing them. #440, the review window. `reviewAppVerdictAtHead` returning null could not tell "this app never reviewed this head" from "the window did not reach far enough to see it", and both readers turned that null into a blocked receipt, which is the false BLOCKED this ticket exists to remove. An old comment claimed truncation was harmless because no review can be newer than the newest; GitHub documents otherwise, since `commit_id` on create-review may name a non-latest commit. Measured on 2026-09-09 against pull request 786: the FILTERED connection returns `totalCount: 112` with `hasPreviousPage: true`, and one commit there carried fourteen reviews inside thirty minutes. The window is a real bound. `resolveReviewVerdict` now walks backwards while records remain, bounded at four extra pages, and both readers call it, so they cannot answer differently. The common path fetches nothing. Past the bound, on a failed page, and with no cursor to walk, the answer is NOT PROVEN rather than absent, recorded as `review.complete` in the artifact and the receipt. #188, the calibration gate. `calibratedFiles()` missed `.agents/skills/*/SKILL.md` entirely, the entrypoints the Codex host actually discovers, so changing the file that decides WHICH prompt runs left every stamped digest untouched and the gate green. Both roots are globbed now, 28 files. Those three pointers also still described tickets as living in Linear, which D57 moved to GitHub. The 90-day backstop was stamp-wide, so reseeding one changed prompt renewed every untouched verdict beside it and ordinary churn held the whole file permanently under the limit. Every verdict carries its own `calibratedAt` now, refused in the future, refused after its own pass, and aged on its own. The reseed pass moved into the repository as `tools/reseed-calibration.mjs`, because a producer living outside it is a producer nobody can rerun: the gate said "recalibrate" and named no command. It carries an unchanged verdict's date forward, so the tool cannot defeat the backstop it feeds. Four bugs of my own, all caught before this push and each now a case: a completeness flag that called an unreachable page a proven absence; a stamp-date comparison written backwards, which a live run caught flagging 27 perfectly ordinary carried-forward verdicts; a resolver imported from `--root`, making the pass unrunnable against anything but a full checkout; and a throw inside a case module, which aborts the whole harness and hides every case after it. Test evidence: node tools/test-tools.mjs ORBIT TOOLS GATE OK exit 0 (1595 pass) node .claude/hooks/test-hooks.mjs ORBIT HOOKS OK exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ * fix(harness): refuse a malformed pagination page, never read it as exhausted One defect, and it is mine: I reintroduced the fail-shut inversion one layer below the one round 9 had just fixed. `PageInfo.hasPreviousPage` is `Boolean!`. Reading it as `=== true` silently turns an absent, null or non-boolean value into `false`, and `false` means "nothing older remains", which `resolveReviewVerdict` then reports as a PROVEN absence. A malformed response could therefore hide a real exact-head approval and be recorded as evidence that none exists. Both parsers now go through one `truncationOf` helper and refuse a page whose `pageInfo` is missing or whose flag is not a boolean. `startCursor` stays NULLABLE by contract, so an absent cursor still just ends the walk as incomplete rather than failing the read. Why the existing cases could not catch it, which is the more useful half. Fourteen fixtures across four modules built `reviews: { nodes: [...] }` with no `pageInfo` at all, including the one commented as copied from a live response. The query SELECTS `pageInfo`, so those fixtures described a response GitHub never sends, and the absent field was exactly the one being mis-read. A fixture that omits what the producer emits cannot fail on how that field is parsed. That is the producer-derived fixture rule TESTING.md already carries, and I did not apply it when I added the selection. All fourteen carry it now, and the live-envelope comment says which half is the 2026-08-12 recording and which half is younger. Three of the fourteen crashed the harness rather than failing a case, which is how they surfaced: a throw in a case module aborts the run and hides every case after it. Metadata, all three raised correctly: - the Parity section claimed no mobile half while the patch bumps three Expo pins in apps/mobile/package.json. Rewritten with the real exemption reason. apps/web/package.json has left the diff: the Next bump reached redesign/main through the drift merge at 9149b58. - `#188` and `#440` were bare, so in this repository they read as pull requests. Now qualified to thomasluizon/orbit-tickets, with a note that they close via complete-ticket.mjs because cross-repo auto-close does not fire. - `Harness Calibration` is still not a required context. That is Thomas's call, so it now has its own section saying so rather than a buried sentence. Test evidence: node tools/test-tools.mjs ORBIT TOOLS GATE OK exit 0 (1605 pass) node .claude/hooks/test-hooks.mjs ORBIT HOOKS OK exit 0 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
thomasluizon
added a commit
that referenced
this pull request
Sep 18, 2026
Round 3 reconciled the four prose sections and left the tool saying something else. `record-readiness.mjs` supplies `pullfrog-approval` itself when protection names no required check, so on `redesign/main` a named substitute reviewer still produces a blocked receipt, and step 9 said a blocked receipt never permits a merge. One run merged and one run stopped from one tree. Step 9 now carries the single exception and the reasoning for keeping the recorder strict: it reads checks and reviews, never a claim in a comment, and a recorder that accepts an unverifiable claim is worse than a written exception a human reads (#429). Step 7 and the queue name the same branch-conditional bar by reference instead of naming Pullfrog. Hard prohibitions and §5.4.1 now say what they decide: the case where Pullfrog produced neither the check nor an APPROVED review, so neither reads as banning the #440 fallback that step 8 and the tool both allow on `main`. Also: three progress signals rather than four, the live `worker` edit is named in the handoff inventory and the step 11 report, and core.md names the file its two pointers live in. #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thomasluizon
added a commit
that referenced
this pull request
Sep 18, 2026
…tract (#1023) * chore: add a Claude headless worker engine and forbid subagent workers An exhausted Codex allowance stopped every code round today. It should not: the launcher already routes on the `worker` key, so a second engine is all that was missing. `.claude/orchestrator.json` now declares `claude` beside `codex`, running Claude Code headless through the same launcher, order file, worktree, timeouts and wake source. Flags were confirmed from `claude --help`, not assumed, and `--dry-run` resolves both tiers. `§5.4.1` of the orchestrate skill states the switch plainly and forbids substituting a Claude subagent for a worker. Measured today: five subagents ran as workers, registered no wake source so the Stop hook read the run as having no continuation, and each had to be told to commit before a handoff was possible, because a subagent dies with its session and leaves its edits uncommitted in a worktree with no record of what they were. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep codex as the committed default worker tools/test-tools.mjs pins the shipped default implementer to gpt-5.6-sol at high reasoning effort, and committing worker="claude" broke both of those assertions. The gate is right: the shipped default belongs in the file, and running on Claude is an operational switch a session makes locally while the Codex allowance is out, not a new default. D95 forbids editing the gate a run is judged by, so the assertion stands and the config yields. Harness went from 61 failures to 1 with this change. The remaining one is create-worktree.mjs hitting EPERM on a lock file under Temp in its concurrency case, which touches nothing here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: isolate the claude worker engine and cover every declared engine The headless Claude engine inherited every user-scope MCP server, because --permission-mode bypassPermissions removes the interactive approval and the PreToolUse hooks match Bash and PowerShell only. --strict-mcp-config with no --mcp-config beside it loads zero servers instead. --output-format stream-json keeps the launcher's log-growth progress signal live, so a worker that spends a long model turn without touching the tree is no longer killed as KILLED_NO_PROGRESS. The installed binary refuses stream-json without --verbose. The config reader validates config.workers[config.worker] only, so a typo in the fallback engine surfaced at the first real launch, after reserveWorkerLaunch had spent one of the two launches the branch cap allows. The gate now resolves every declared tier of every declared engine. Refs #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: say what the claude worker engine means when an operator sets it core.md loads on every turn and read "Claude never edits code", full stop, so a session that hit an exhausted allowance read that first and stopped the night. That is the ending D104 exists to prevent. It now points at the fallback. Five corrections inside the section the tree contradicted: the launcher's pid is the wake source with the worker's pid beside it, a subagent does NOT end the night silently because the Stop hook blocks, the --dry-run is an argv check rather than a preflight, the calibration stamp goes stale for the duration of the switch and nobody reseeds it, and the worker edit is uncommitted with git checkout -- .claude/orchestrator.json as its revert. The staleness trap in assertNotStale is named beside them, because it fires on the config edit itself. The reviewer clause turns on the word quietly. On protected main no substitute may stand in for pullfrog-approval. On unprotected redesign/main no such check exists to forge, so a separate-agent review is acceptable when the pull request comment names the substitution and lists checkable evidence. The skill is calibrated, so its verdict is rewritten for the new text and the stamp reseeded: 1 entry renewed, 31 carried forward, worker still codex. Refs #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: make the merge bar say one thing in all four places Round 3 on #598. The reviewer clause 5.4.1 gained in round 2 contradicted three other sections of the same file, so a night run reached a different merge answer depending on which section it read. - Step 8, Hard prohibitions and 5.4.1 now state one branch-dependent rule: absolute on protected main, a named separate-agent review admissible on unprotected redesign/main, and a green check alone never an approval. - 5.4.1 records that the clause came from a written instruction at entry, not from a contract this run granted itself. - Pin the claude engine's command and both model ids by name, because resolveWorkerInvocation never reads command and reads the model from the block the loop compares against. claude-opus-4-8 is a real id, so that typo would run a whole night in silence. - Name the tools-harness red beside the calibration red, by assertion, and say neither licenses editing a test. - Correct the assertNotStale headline, the stream-json rationale, the launch-cap comment, the launcher line reference and the missing --tier mechanical dry run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(tools): pin the MCP absence and the unprotected review verdict The claude engine assertion checked that `--strict-mcp-config` is present and never that `--mcp-config` is absent, so adding the flag left every user-scope MCP server loading into a worker at bypassPermissions with the test still green. Assert the absence. `record-readiness.mjs` supplies the `pullfrog-approval` axis itself when protection names no required check, which is the whole reason an unprotected base is not a free pass (#429). That rule now lives beside the reader it feeds as `reviewChecksFor`, so a test drives the real rule, and the receipt verdict it produces on `redesign/main` with no review is pinned as CI_STALE alone. #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: make the tool and the prose agree on the merge bar Round 3 reconciled the four prose sections and left the tool saying something else. `record-readiness.mjs` supplies `pullfrog-approval` itself when protection names no required check, so on `redesign/main` a named substitute reviewer still produces a blocked receipt, and step 9 said a blocked receipt never permits a merge. One run merged and one run stopped from one tree. Step 9 now carries the single exception and the reasoning for keeping the recorder strict: it reads checks and reviews, never a claim in a comment, and a recorder that accepts an unverifiable claim is worse than a written exception a human reads (#429). Step 7 and the queue name the same branch-conditional bar by reference instead of naming Pullfrog. Hard prohibitions and §5.4.1 now say what they decide: the case where Pullfrog produced neither the check nor an APPROVED review, so neither reads as banning the #440 fallback that step 8 and the tool both allow on `main`. Also: three progress signals rather than four, the live `worker` edit is named in the handoff inventory and the step 11 report, and core.md names the file its two pointers live in. #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(calibration): reseed the orchestrate stamp and reconsider its verdict The merge bar moved out of prose alone and into step 9, where `record-readiness.mjs` decides. The verdict says what that costs a reader: the recorder cannot verify a substitution written in a comment, so it blocks on the review axis by design, and the run has to tell that one expected verdict apart from a real blocker reported under the same name. One entry renewed, 31 carried forward. #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct the new citations and say how the night ends `reviewChecksFor` moved the rule, so step 9 names its real home and the line the recorder calls it from. The exception paragraph now also says what happens after it: a merge admitted there closes the pull request exactly as a READY one does, and when it does not apply the run records the `blocker` string that the Stop hook already accepts as a legitimate BLOCKED ending. Reseeded after the edit: one entry renewed, 31 carried forward. #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(orchestrate): let a merged pull request end an unattended night A pull request merged under the step 9 exception could never end the run that merged it. The Stop hook reads the receipt and never the pull request state, and that receipt stays CI_STALE forever because pullfrog-approval will not publish on an unprotected base. Driven against checkSleepStop: a merged row with no blocker returned block:true, the 2026-08-08 deadlock reached after a successful merge. Adds `merged` as the third ledger disposition beside READY and BLOCKED. The bar is the merge commit sha, a fact the run records and a reader can check on GitHub. It is sticky where a blocker is not, because a merge cannot be undone. Also corrects two things the tree contradicted. The expected-FAIL count for the worker switch is four, not two: the earlier reading used --only orchestrator-config, which cannot see the two launch-worker pins. And the MCP absence was asserted on workers.claude.args, while the worker's real argv is the engine array concatenated with the tier's, so --mcp-config in a tier's args passed the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(calibration): reseed the orchestrate stamp for the third disposition Sharpens the CI_STALE signature in step 9, then reseeds. The signature is delivery's ci.checks.pass true beside the receipt's ci.green false, and only a required check absent from the rollup entirely reads that way: one that ran and failed lands in rollup.failing, and a running one in rollup.pending, so either drops delivery's own pass. The verdict text is rewritten before the stamp, so the reseed is not re-blessing old reasoning about new prose. worker stays codex, so workerMoved is false and the other 31 dates carry forward untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(orchestrate): require a real sha for a merged row and announce that ending `merged` accepted any non-empty string, so copying the skill's own template and leaving `<merge commit sha once it is merged, or absent>` unfilled reported an UNMERGED pull request as a finished night, printing nothing. Driven against `checkSleepStop`: that literal and the bare word "yes" both returned null. Both predicates now test `/^[0-9a-f]{7,40}$/`, in the Stop hook and in the recorder, and a new assertion pins the two copies to the same literal so the lenient direction cannot drift back. A merged-only completion returns a terminal MERGED banner naming each pull request and its sha, and `require-wake-source.mjs` writes it beside the BLOCKED one, so the sha reaches a reader. Step 11 now asks for the disposition per pull request, the readiness-receipt assertion compares against the literal `pullfrog-approval` rather than the constant it imports from the module under test, and both SKILL.md and NEXT.md name the load-sensitive `launch-worker.mjs` CPU test so a fifth FAIL is checked against that name first. Refs #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(calibration): reseed the orchestrate stamp for the checked merge sha The skill gained a shape-checked `merged` sha, a MERGED terminal banner, the disposition per pull request in step 11, and the note that its fifth FAIL may be the load-sensitive CPU test rather than a defect. The verdict now names the call that change asks for: which self-asserted fact stays honest is the one a reader can check. `worker` stays `codex`, so `workerMoved` is false and the other 31 verdicts carry forward untouched: `stamped 32 file(s) ... 1 verdict(s) renewed, 31 carried forward`. Refs #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: attribute the load-sensitive FAIL to the head it was measured at This round did not reproduce `ORBIT TOOLS GATE FAILED (1)` on the CPU test: the full run passes it at 1824 assertions. The measurement belongs to the review at head `4d590110`, so both documents now say so rather than reading as this run's own observation. The mechanics stay confirmed from the tree. Refs #598 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stage 4 of the harness-upgrade plan — the on-demand cross-model
/second-opinionskill, wired into/pr-review.A different model (GLM-5.2 on OpenCode Go) independently judges each Critical code-review finding that survives the adversarial skeptic. Agreement corroborates the finding; disagreement marks it
CONTESTEDand surfaces both verdicts — it never forces a merge. On-demand cross-model diversity reserved for high-stakes calls (no standing consensus voting, perresearch.md).What's in it
.claude/skills/second-opinion/—SKILL.md+ a deterministicsecond-opinion.mjshelper:opencode run --model opencode-go/glm-5.2 --format jsonvia stdin (dodges Windows argv length/quoting + the.cmdEINVAL).type:textevents → the inner verdict JSON, always exits 0, degrades toUNAVAILABLEon absent-binary / error-event / insufficient-balance / timeout / unparseable.opencode modelscache is stale (lists onlyglm-5/glm-5.1);glm-5.2is live on Go and opencode routes it to the Go endpoint./pr-reviewPhase 6 step 2 (Critical survivors only, after the adversarial skeptic): AGREE → corroborated · DISAGREE →CONTESTED(both verdicts inline, never forces merge) · UNSURE/UNAVAILABLE → finding stands. CONTESTED never changes the deterministic NEEDS-WORK recommendation.UNAVAILABLE→ the review stays byte-identical to today (mirrors the audit skills' CI-fallback)..gitattributeseol=lf gate for skill.mjs; plan Stage 4 checkbox checked.Verification
{status:OK, verdict, confidence, reasoning}).node --check).Live status
opencode-go/glm-5.2currently returnsInsufficient balance(a funding state on the Go plan, not a bug) → the helper'sUNAVAILABLEpath. Fund Go/Zen at the billing URL and it goes live with zero code changes.Paired with orbit-api (the
/pr-reviewverification-twin) — cross-linked below.🤖 Generated with Claude Code