A failed-review verdict is permanent, but the branch it judged is not - #575
Draft
agent-relay-code[bot] wants to merge 4 commits into
Draft
agent-relay-code[bot] wants to merge 4 commits into
agent-relay-code[bot] wants to merge 4 commits into
Conversation
The hosted capability loader accepts exactly one Software Factory base: the
bytes of examples/software-factory/software-factory.flow.ts, named by
SOFTWARE_FACTORY_SHA256, with the identity it assigns hard-coded beside it.
This branch changed that flow and bumped its header to 2.0.23 without moving
either pin, so loadHostedExtensionRuntime rejected the base the SDK ships with
and tests/babysitter-native-extension.test.ts could not load at all:
Error: Hosted capability isolation accepts only the reviewed Software
Factory base source.
❯ baseAt src/hosted-extension-runtime.ts:239:13
Test Files 1 failed (1)
Tests 41 skipped (41)
Repin both to the reviewed source:
$ git show HEAD~1:examples/software-factory/software-factory.flow.ts | sha256sum
49c993220b9c34fab2d4b0e51911656f62b8b657f534d988691960d45bb9d9b6 -
$ sha256sum examples/software-factory/software-factory.flow.ts
ee56899fcb5c0a968d845620db3d4229673a3b732dd4d6131ab43b81822bf97b examples/software-factory/software-factory.flow.ts
The check itself is untouched and no test was weakened: every other source is
still refused, as the unmodified rejection cases in
tests/hosted-base-snapshot.test.ts show (18 passed). The forged-digest case in
babysitter-native-extension.test.ts keeps its meaning by carrying the new pin —
it poisons Hash.digest to return the *expected* value and asserts the captured
intrinsics are used instead. docs/BABYSITTER-CATALOG-HANDOFF.md now says the
flow bytes and the two pins move together.
After the repin the suite runs: 40 passed, 1 failed — the bubblewrap isolation
case, which cannot start a namespace on this machine for reasons unrelated to
this branch (.relayflow/repair-notes.md).
Mutation-verified, both pins, each restored from a saved byte copy with cmp:
$ sed -i "s/ee56899…/49c9932…/" src/hosted-extension-runtime.ts
$ vitest run tests/babysitter-native-extension.test.ts
Tests 41 skipped (41) # suite fails to load
$ cp /tmp/hosted-extension-runtime.ts.fixed src/hosted-extension-runtime.ts
$ cmp … && echo restored byte-for-byte
restored byte-for-byte
$ vitest run tests/babysitter-native-extension.test.ts
Tests 1 failed | 40 passed (41)
$ sed -i "246s/'2.0.23'/'2.0.22'/" src/hosted-extension-runtime.ts
$ vitest run … -t 'composes onto Software Factory'
- "version": "2.0.23" + "version": "2.0.22" # identity test fails
$ cp /tmp/hosted-extension-runtime.ts.fixed src/hosted-extension-runtime.ts
$ vitest run … -t 'composes onto Software Factory'
Tests 1 passed | 40 skipped (41)
Full commands and output are in summary.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The report claimed the verdict-classification mutations were "restored using a
saved byte buffer with equality asserted" but pasted no restore evidence, which
is exactly the shape AGENTS.md rejects. Re-run all of them at this head with
the diff, the failure, the cmp, and the pass captured literally, and add a
fourth mutation for the 40-hex reviewed-head guard, which had no mutation check
before:
M1 scope guard moved behind the GitHub arm -> 'rejects a forged scope for
github' fails, 'local' still passes
M2 classifier's final else BLOCKED -> PASSED -> 'fails closed for empty
unverified' fails
M3 count != 1 arm BLOCKED -> PASSED -> 'fails closed for no verdict' fails
M4 /^[0-9a-f]{40}$/ -> /^.*$/ -> all three 'rejects malformed head' cases fail
Each restored with cmp asserting byte equality and re-run green; the flow file
still hashes to the pinned ee56899…, so the hosted reviewed-base pin is intact.
No production code changed here. The README loses one stray double blank line
in the prose this branch added.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Bind draft review verdicts to the commit they judged
Draft PRs from the canonical Software Factory flow now name their reviewed
40-hex head, include a single machine-readable review marker, and explain that
a new head supersedes the verdict. Missing verification prerequisites use
NOT VERIFIED, name the prerequisite, and end with needs_human (exit 3) after
opening a draft. Defects, absent or contradictory verdicts, and empty unverified
markers remain BLOCKED / step_failed (exit 1). Completion details identify the
head and publication state. Hook-blocked drafts use the same scope contract.
Publication rejects malformed heads and duplicate/forged scope markers before
push, including GitHub inputs. Passed PR body formatting is preserved.
Regression coverage exercises these cases using the real /bin/sh body-generation
and validation commands; the harness now throws for unhandled commands.
The flow header is bumped to 2.0.23, and the hosted loader's reviewed-base pins
move with it (next section). No workflow files were changed.
The reviewed-base pins move with the flow
packages/sdk/src/hosted-extension-runtime.tspins the exact Software Factoryflow bytes the hosted capability sandbox will accept, plus the identity it
assigns to that base. Both are functions of
examples/software-factory/software-factory.flow.ts:Changing the flow and leaving the pins behind makes the loader reject the base it
ships with, which is exactly what it should do — and it took the whole
babysitter-native-extensionsuite down with it.SOFTWARE_FACTORY_SHA256andthe assigned version are therefore updated to the new bytes and the new
2.0.23header, and
docs/BABYSITTER-CATALOG-HANDOFF.mdnow states that the two movetogether. The check itself is unchanged: any source other than the reviewed one
is still refused, as the untouched rejection regressions in
tests/hosted-base-snapshot.test.tsstill prove.Scope and limitations
This implements reviewed-plan.md's record-the-head portion and three-verdict
protocol in the tracked canonical flow. It does not amend stale PR comments,
clear drafts, rerun reviews on push, or backfill the seven PRs. The future
resident shepherd (examples/babysitter, not yet ready for unattended deployment)
can consume the new marker. The running Garden flow is not tracked here:
the equivalent change must be transplanted into its reviewBlockedCommand and
review.clean check. This does not claim the running Garden is fixed.
22 tests in three files still fail on this machine, all of them the bubblewrap
sandbox failing to start, which no code change here can clear; the numbers and
the evidence are below and in
.relayflow/repair-notes.md. Onelive-kernelcase that drives the real Claude analyzer is skipped
(
RELAYFLOWS_ALLOW_ANALYZER_SKIP=1, as CI sets it), so this is not gate-2acceptance evidence.
Captured verification — repair pass
The whole repository check,
.relayflow/check.sh, which mirrors the fourPR-triggered workflows (
cloud-runtime-artifact.yml,surface-package.yml,schema-publish.yml's validate job, and the offline half ofreview-swarm-wrapper-guard.yml):Kernel workspace: 27
test result: oklines, 278 tests, 0 failed. Surfacepackage gate: 52 source tests,
PACKED_RUNTIME_REFUSAL_OK,PACKED_TYPESCRIPT_OK, 34 packed-consumer tests. Schema: regenerated twice,git diff --exit-codeclean, 79 schema tests pass. Review-gate parity:lens-parity-check: PASS,lens-cli-parity-check: PASS,21 passed, 0 failed.Every one of the 22 failures is the bubblewrap sandbox — the machine cannot
create unprivileged user namespaces
(
kernel.apparmor_restrict_unprivileged_userns=1,/proc/sysis a sysbox FUSEmount that
sudo sysctl -wcannot write, and Debian's bubblewrap has no setuidsupport). Full diagnosis, including the probes that rule out every workaround,
is in
.relayflow/repair-notes.md.The two suites this change actually touches:
The one remaining failure there is
runs the exact published 2.0.26 native bytes in the isolated capability path,the bubblewrap case. Before the pin update the same file could not load at all:
Tests 41 skipped (41),Error: Hosted capability isolation accepts only the reviewed Software Factory base source.Mutation checks — the reviewed-base pins
Each mutation changed one pin, ran the suite, then restored the file from a saved
byte copy with
cmpasserting byte equality, and re-ran.Captured verification — implementation pass
Before implementation, with only the harness made strict:
Final selected suites:
Test TypeScript compilation:
Examples typecheck produced the same errors before and after implementation
in an untouched file (neither run passes):
Mutation checks — scope placement, verdict classification, head validation
Re-run at this branch's head against the committed flow. Each mutation edits
examples/software-factory/software-factory.flow.ts, runs the named regression,restores the file from a copy saved before the first mutation
(
cp examples/software-factory/software-factory.flow.ts /tmp/flow.fixed), provesthe restore with
cmp, and re-runs the same regression. Failure and pass areboth captured below;
…/vitestispackages/sdk/node_modules/.bin/vitest, runfrom
packages/sdk. Output is filtered to the result lines(
grep -E "✓|×|Tests |Test Files |AssertionError|Expected:|Received:").M1 — the scope guard must not sit behind the GitHub branch. The standalone
guard becomes an
elifafter the arm that already answersvalidfor GitHubsources, so a GitHub PR body would never be scope-checked:
The
localcase passes under this mutation and thegithubcase does not,which is the point: the guard's placement, not its existence, is what makes it
reach a GitHub body.
M2 — the classifier's final
elsemust stay BLOCKED.else echo BLOCKEDbecomes
else echo PASSED, which is the arm an emptyreview.unverifiedfallsthrough to:
M3 — "not exactly one verdict" must stay BLOCKED. The
count != 1armbecomes
PASSED; that is the arm a silent adversary (no verdict file at all)takes:
M4 — the reviewed head must be validated before it reaches a command. The
40-hex test is widened to match anything:
After all four, the flow file is the committed file — which is the restore proof
that outlives
/tmp:(The one modified file is a stray double blank line removed from the README
prose added by this branch; the flow, the tests and the pins are untouched.
summary.md— this file — is modified too, which is why the status above isscoped to the code paths.)
Re-verified at this head
The single failure is the bubblewrap case described above and in
.relayflow/repair-notes.md; it fails identically on an unmodified checkout ofthis machine and no change here can clear it.
Checks
The checks fail on the base commit too, so these failures were not introduced by this change: they come from the repository itself or from the environment the checks ran in. This pull request is a draft until someone looks.
What ran (.relayflow/check.sh)
Output on this branch (last 80 lines)
Output on the base commit (last 80 lines)
What the repair agent found
.relayflow/check.shon this machine — what failed, and why.relayflow/check.shmirrors the four PR-triggered CI workflows. Run on thisbranch it stopped on the very first step, and the failures behind it split into
three kinds: missing setup (fixed in
check.sh, which is not committed),a bug in this branch (fixed and committed), and one environment limit that
cannot be fixed here (left failing, documented below).
Final state of a full run: 22 failing tests in 3 files, every one of them the
bubblewrap sandbox failing to start. Everything else — 3194 SDK tests, the
kernel workspace suite, the surface-package gate, the schema codegen/parity job
and the offline review-gate checks — passes.
1. Outside our control: bubblewrap cannot create namespaces here
Failing:
tests/hosted-extension-isolation.test.ts(13),tests/hosted-extension-protocol.test.ts(8), and one case intests/babysitter-native-extension.test.ts(
runs the exact published 2.0.26 native bytes in the isolated capability path).Every failure reads
Hosted extension sandbox exited without a valid completion (exit 1): bwrap: ….These tests exercise the real production namespace command
(
packages/sdk/src/hosted-extension-sandbox.ts), so they need a workingunprivileged
bwrap. CI provisions it by clearing an AppArmor knob on itsephemeral VM (
cloud-runtime-artifact.yml:124-137). That knob is not writablehere —
/proc/sysis a sysbox FUSE mount — and there is no other route to thecapability:
Handing the unprivileged user the capability another way does not work either —
Debian's bubblewrap is built without setuid support:
Both probes were reverted:
Running the suite as root only moves the wall: the smoke command succeeds, and
the real sandbox then fails one step later, because the container will not let a
nested namespace mount
proc:Left failing. No change to this branch's code can clear it, and the tests are
right to fail closed rather than skip: they are the isolation gate. The bwrap
smoke in
check.shwas therefore made advisory (it prints a note and continues)so the rest of the run is reachable; the tests themselves were not touched.
2. Missing setup: Bun 1.3.6 where CI pins 1.4.0
tests/authored-node-runtime.test.tsfailed atbeforeAll:The assertion is correct — that suite builds and runs the packed standalone CLI,
and CI pins
bun-version: "1.4.0"in all four workflows. This machine ships1.3.6 on
PATH.check.shnow installs the pinned Bun beside it and prefers it,which is what
oven-sh/setup-bun@v2does. After that the suite passes:3. Missing setup: an inherited
"type": "commonjs"above the checkoutEight
tests/live-kernel.test.tscases failed withoutput: nullandverification: { gate: 'execution', verdict: 'fail' }, orTypeError: Cannot read properties of null.Cause: Node decides whether an extension-less file is ESM or CommonJS from the
nearest ancestor
package.json. This sandbox has one at/home/daytona:The agent-CLI fixtures in
testdata/preflightare extension-less ESM withtop-level
await. Under that inherited"commonjs"they run, exit 0, and printnothing, so the worker records a null agent output:
A GitHub runner has no
package.jsonabove the workspace.check.shrestoresthat by writing a neutral
{}manifest immediately outside the repository(
$(dirname "$repo_root")/package.json) — never inside it: this repodeliberately has no root
package.json, and adding one would change what CIchecks. With the shim the whole file passes:
4. A bug in this branch (fixed and committed)
tests/babysitter-native-extension.test.tsfailed as a whole suite:SOFTWARE_FACTORY_SHA256and the assigned base version inpackages/sdk/src/hosted-extension-runtime.tsare bytes-of-the-flow pins:This branch changed that flow and bumped its header to 2.0.23 without moving the
pins, so the hosted loader rejected the base it ships with. Both pins were
updated to the new bytes and version; the commit message records the mutation
check. This is the only committed fix.
What was changed in
check.sh(not committed — it is gitignored)sysctl -wand thebwrapsmoke are advisory instead of fatal,with a comment naming §1 as the reason.
package.jsonshim above the repository is written if absent (§3).check.shareindependent jobs in CI — one failing job never stops the others — and with
§1 permanent here, aborting hid three whole workflows' worth of checks behind
it. The status is remembered and the script still exits 1 at the bottom:
FAILED: the SDK suite above exited nonzero (sections 2-4 still ran; see their output).Fixes #571
Summary by cubic
Binds Software Factory draft review verdicts to the exact commit they judged, so a failed review stays final without condemning a superseding head.
Behavior
needs_human(exit 3) after opening a NOT VERIFIED draft; defects, contradictory or absent verdicts, and empty unverified markers remain BLOCKED /step_failed(exit 1).Follow-on
packages/sdk/src/hosted-extension-runtime.tsto the new flow bytes.Written for commit 5d65052. Summary will update on new commits.