review: fix the out/ artifact upload (allowed-paths must match staging-relative paths)#243
review: fix the out/ artifact upload (allowed-paths must match staging-relative paths)#243jwbron wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 4bd445c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
63c8e1e to
6a5adf7
Compare
|
Restacked: this PR is now based on main and carries only the fix commit (cherry-picked as 6a5adf7; content unchanged, verified by tree diff). Rationale: the fix is behavior-neutral infra with two downstream consumers (#225's live counters, the A/B runner's claim auditing), so it should merge first and gains nothing from waiting behind the eval stack or from A/B coverage. If you have local state on jwbron/review-out-artifact-upload, reset to origin before continuing. Batch two's behavior PRs (re-review accountability, out-of-lane handoff, dispatch-tax trim) should still base on jwbron/review-trial-skill, which now sits on top of the full A/B chain, so each behavior PR gets a per-push Review Eval A/B report. |
…oad (allowed-paths must match staging-relative paths)
6a5adf7 to
5dd182b
Compare
|
Restack, final topology (supersedes my earlier comment): this PR is back between the eval stack and the behavior PRs, i.e. main -> #233 -> #234 -> #236 -> #237 -> #238 -> #243 -> #244 -> #245. Two things happened: (1) the whole eval chain had been cut from a stale main that predated review@1.4.0 (missing #228/#230), which would have made every A/B arm and every stacked behavior PR run a pre-v1.4.0 prompt; the chain is now refreshed with main via bottom-up merges (no history rewrites on the chain itself). (2) This branch and the two behavior branches were rebuilt on the refreshed chain (cherry-picks 5dd182b / 491a983 / 7b5318c; content unchanged). If you have local state on any of the three batch-two branches, reset to origin before continuing. The dispatch-tax trim PR should base on jwbron/review-out-of-lane. |
|
Field verification: webapp preview run 29051817727 (Khan/webapp#40736, preview scaffolding pinned to this branch) uploaded the |
Review live A/BBaseline:
Improvements (candidate caught, baseline missed)
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
…-skill' into jwbron/review-out-artifact-upload
…-skill' into jwbron/review-out-artifact-upload
Summary
First of five PRs in the fold-in batch two stack, stacked on #238 (
jwbron/review-trial-skill, the top of the open eval stack). Stack order: this PR, then re-review accountability, out-of-lane handoff, dispatch-tax trim, and quoted-rule rendering. The top of the batch-two stack will bejwbron/review-skill-rule-quote; downstream work (the re-review mode dial, the webapp preview runs) should base on that branch.On gh-aw v0.81.6 the safe_outputs job of every review run annotates
ERR_VALIDATION: upload_artifact: no files matched the selection criteria, and none of the out/ files (sub-agent outputs,pre-existing.json) reach the run artifacts; that blocks post-hoc claim auditing and everything that reads run artifacts (the #225 live counters, the A/B runner).Root cause (traced through the gh-aw v0.81.6 source and the run logs of the v1.4.0 re-run lifecycle, runs 29044383251 / 29045581242 / 29046928431 of
review-preview.lock.ymlon Khan/webapp#40730):upload_artifactcalled with{"path": "/tmp/gh-aw/review/out/"}.safe_outputs_handlers.cjsuploadArtifactHandler) copies the directory into its staging area under the directory's basename and rewrites the recorded path to the staging-relativeout(the JSONL entry is{"type":"upload_artifact","path":"out"}).upload_artifact.cjsresolveFiles) lists the staged files relative to the staging dir (out/<agent>.json) and filters them againstallowed-pathswith a fully anchored regex (glob_pattern_helpers.cjs). A staging-relative path can never match the absolute pattern/tmp/gh-aw/review/out/**, so the filter empties the candidate set on every run and the handler returns the observed ERR_VALIDATION. With an absolute pattern, no tool input can succeed; upstream main (checked at f2bff5d5) has the same behavior.The fix is config-side:
allowed-pathsnow lists the staging-relativeout/**, keeping the absolute form alongside it (the filter is an OR across patterns) in case a future gh-aw release matches against the original path. The Step 9 prompt keeps the absolute-directory-path instruction (it is correct) but now explains the actual staging semantics instead of the outdated rationale, and adds one step: copyclaims.jsonintoout/before uploading, so the artifact carries the validator's input alongside its verdicts (claim-validator.json) and the provenance gate's set-asides (pre-existing.json), completing the claim-audit trail.Failure scenario fixed: a reviewer investigating why run 2 of the lifecycle left three blocking threads unacknowledged has no
out/thread-reconciler.jsonto consult; the #225 live counters find no artifact and report nothing; the A/B runner cannot score producer outputs. After this change theoutartifact uploads with all staged files.Note for reviewers: this branch was rebuilt on the refreshed eval chain (see the restack/topology comments below); the fix now rides a single cherry-picked commit on top of #238's branch, content unchanged from the original.
One adjacent gap surfaced while verifying, not fixed here: the shipped review-v1.4.0 tag still pins the
pre-agent-stepslib checkout atref: review-v1.2.2(review.md line 208), which predateslib/provenance.ts; a consumer compiling plain review.md@review-v1.4.0 gets a lib checkout that cannot run the provenance CLI (the gate fails open with a note). The webapp preview lock pins a newer ref, which is why the lifecycle runs were unaffected. Worth folding a ref bump into the release process rather than hand-bumping in this stack.cc @jeresig
Testing
npx vitest run workflows/review: 457 tests green.globPatternToRegex("/tmp/gh-aw/review/out/**")rejectsout/claim-validator.json,out/pre-existing.json,out/claims.json;globPatternToRegex("out/**")matches all three.safe-outputs-upload-artifactshandoff artifact of run 29046928431 contains all 11out/*.jsonfiles), so the only failure is the pattern filter this PR changes. An end-to-end run rides the webapp preview recompile that the downstream preview-runs work performs against this stack.