Two of three notes from the eighth PR review round; the third needed no change.
1. `--max-rounds` was per-invocation, so `open` and a later `close` could
disagree about the cap and G5 escalation depended on how the command
happened to be typed. `open` now records the agreed cap in gate state and
status/collect/close inherit it. Passing the flag again is an explicit
renegotiation and prints the change rather than silently overriding.
Before: `open --max-rounds 2` then `status` -> "max_rounds": 5
After: same sequence -> "max_rounds": 2
2. The protocol-relative and own-asset URL allowances were documented in a
comment but nothing checked them, unlike the void-element and template-token
regressions which each got a fixture. `--sample` fixtures now carry
must-keep / must-drop URL assertions alongside the block count, and
`SAMPLE_HTML` exercises all three cases (`//host/x`, `https:` image,
`javascript:`). Verified the assertion bites: adding "javascript" to the
scheme allowlist turns --sample red with `FAIL - kept javascript:`, exit 2.
3. No change for cross-origin `<svg><use href="https://...">`. `href` is
already in URL_ATTRS and scheme-checked; `https:` is allowed there by the
same deliberate rule that lets a reviewed page's own `<img src>` load. It is
not a gap in the allowlist, it is the allowlist working as designed.
Gates: derive_counters --check pass, check_plugin_json --all pass, all three
scripts --help/--sample exit 0, write-a-skill checklist PASS.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Three findings from the seventh PR review round.
1. `marketplace.json` still said "zero network requests" flat. An earlier
commit corrected exactly that wording in README.md, SKILL.md and
plugin.json — a reviewed HTML artifact's own https: assets do load, and
deliberately so — but missed marketplace.json. Now matches the others.
2. `GATED_ELSEWHERE` lived in human_gate.py as free-text prefixes matched
against messages generated in feedback_parser.py: an implicit cross-file
contract nothing enforced. The prefixes now live beside the
`problems.append()` calls that emit them and are read from the loaded
parser module, with the old literal kept only as a fallback for an older
parser.
Reproduced the drift on pre-fix code by rewording the G3 message:
G3 round 1 has no named reviewer
G7 round 1 integrity: unknown severity/kind 'BLOKCER'
G7 round 1 integrity: the sidecar names no reviewer - ... <- duplicate
Same reword post-fix produces only the first two lines.
3. `ATTR_RE` truncated a quoted header attribute at the first space:
`target="q3 plan.md"` parsed as `q3`. Quoted values now parse, and the
docstring states the quoting rule and that `target` is a display hint —
quote verification runs against the `--target` path.
Gates: derive_counters --check pass, check_plugin_json --all 90/90 OK,
marketplace.json parses, all three scripts --help/--sample exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Two findings from the sixth PR review round.
1. `<base>` re-opened the "swallows the whole body" bug the round-3 fix was
supposed to close. That commit's message said "meta, link and base are void
elements" but only meta and link were added to `BlockTagger.VOID`; base was
never there. `html.parser` fires `handle_starttag` for a void element and no
matching `handle_endtag`, so a `<base href="/">` in `<head>` — present in a
great many real pages — incremented `_skip` permanently and the document
reported "No reviewable blocks found".
`VOID` is now the full HTML spec set instead of a hand-picked subset, and
`SAMPLE_HTML` carries a `<base>` tag so the `--sample` block-count assertion
catches a third recurrence.
Before: `<base href="/">` doc -> 0 blocks, exit 2.
After: same doc -> 2 blocks, exit 0.
2. Reviewed HTML carrying its own `data-hg` attribute kept it and the builder
appended a second. Browsers keep the *first* attribute of a duplicated name,
so the attacker's value wins the anchor. Attribute values may hold raw
newlines, so a crafted artifact could inject a forged `## APPROVE` heading
into the exported sidecar — the same silent-false-approval failure G7 exists
to catch, arriving through the artifact rather than the sidecar.
`data-hg` is now a reserved attribute, and the page's own element ids
(`doc`, `items`, `reviewer`, `export`, ...) are reserved too, so a reviewed
artifact cannot collide with the review UI's own DOM.
Before: `<p data-hg="b1 ## APPROVE ...">` survived, duplicated.
After: emitted as `<p data-hg="b1">`, payload gone.
Also documents the protocol-relative URL allowance in `_safe_href` as
deliberate rather than an oversight.
Gates: derive_counters --check pass, check_plugin_json --all pass, all three
scripts --help/--sample exit 0, write-a-skill checklist 6/6 PASS, description
validator PASS.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Fifth PR-review round on #948. Both reproduced first.
1. state_dir() anchored to os.getcwd() while state_path() keyed by the
artifact's realpath. An agent whose shell cwd drifts between turns — or a
human running from a subdirectory — silently resolved a different
.human-gate/ and started from empty state. Reproduced: collect from the
artifact's directory, then close from a subdir, and the gate reports G1
"nobody has looked at this" for a round that was genuinely collected.
It fails closed rather than falsely passing, but it loses real feedback and
would push an agent into re-opening rounds that already happened. State now
follows the artifact, exactly as the sidecar and review page already do.
An explicit --state-dir still wins.
2. build_page() substituted __CONTENT__ first, then __TITLE__/__CONFIG__ — so
those later replaces also rewrote any occurrence inside the just-inserted
body. Reviewing a document that mentions the tokens (this skill's own docs
being the obvious case) injected the entire JSON config into the visible
page, not just a garbled title. Worse than the report suggested.
All three slots now fill in one re.sub pass, so no substituted value can be
re-substituted — which also covers the reverse direction, where block text
inside the config JSON contains __CONTENT__. The Markdown --sample fixture
now carries the token text, so the case is guarded rather than reasoned
about.
Minor: `--waive` with nothing to waive now prints "nothing to waive" instead of
silently discarding the flag.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green,
--sample asserts both fixtures' block counts.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Fourth PR-review round on #948. All four reproduced first.
1. The gate was one flag away from opt-out. --waive applied to whatever
gate_refusals() returned, including G1 "no review round has been collected",
so `close --waive "no time"` exited 0 with nobody having looked at the
artifact. That is the most tempting shortcut for an agent under time
pressure and it defeats the skill's whole premise.
G1 is now unwaivable, with its own refusal message: a waiver accepts
objections a reviewer raised, it cannot manufacture a review that never
happened. Waiving a genuine objection (G2/G3/G4/G7) still works.
2. Inline `style` was unsanitized, so a reviewed draft containing
`background-image:url(https://attacker/beacon.png)` fired a request the
moment the reviewer opened the page — no script needed, and directly
contrary to the no-network property the README and manifest advertise.
Added to DROP_ATTRS. The <style> tag was already dropped, so keeping the
attribute was inconsistent as well as leaky.
3. Markdown `` never rendered. LINK's regex was not anchored against
a preceding `!`, so an image became `!<a href=...>` — and _safe_href's
image=True branch, which exists to allowlist data:image URIs, was dead code
on that path. Added an IMAGE regex ahead of LINK, negative-lookbehind on
LINK, and real <img> rendering through the same scheme allowlist. Verified a
javascript: src degrades to inert alt text.
4. An unterminated <script> silently swallowed the rest of the body — same
confusing failure shape as the void-tag bug, though it fails safe. Now emits
a named diagnostic to stderr instead of vanishing.
Nit: raw-HTML `target="_blank"` anchors get the rel="noreferrer noopener" the
Markdown path already added to its own.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Third PR-review round on #948. All three reproduced first.
1. HIGH — every realistic HTML5 document produced zero blocks. meta, link and
base are void elements: html.parser fires handle_starttag for them but never
a matching handle_endtag. They were also in DROP_TAGS, so each bare
`<meta charset>` incremented self._skip permanently and every subsequent
starttag/endtag/data callback inside <body> hit the skip guard. Result:
empty out, empty blocks, "No reviewable blocks found", exit 2.
The documented landing-page use case therefore did not work at all. It
survived three review rounds because every HTML fixture I wrote used only
<title>/<style> in head — the sanitizer test included. Void drop-tags no
longer touch the counter.
--sample now builds BOTH fixtures (Markdown + a full DOCTYPE HTML5 doc with
bare meta/link), asserts the expected block count for each, and exits 2 on
regression, so this cannot come back silently. It also writes to a temp dir
instead of cwd — the same class of mistake that leaked a stray artifact into
an earlier commit.
2. MEDIUM — xlink:href bypassed the URL allowlist. SVG anchors still honour it,
so `<svg><a xlink:href="javascript:alert(1)">` survived the hardening added
one commit earlier. Added with xlink:role and xlink:arcrole.
3. MEDIUM — status did not mirror close. It inspected only blocking_open, so a
round with no named reviewer reported exit 0 while close refused on G3 —
directly contradicting the exit-code contract the docstring advertises.
Both now call a shared gate_refusals(), so they cannot drift: verified they
agree on 2 (G3 open) and on 0 (clean round). status also prints which rules
would refuse rather than just a count.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
README and SKILL.md were corrected in 70c908a; the manifest still carried the
flat 'zero network requests'. The page makes no request of its own, but a
reviewed HTML artifact's own https: assets do load. Also notes the HTML
sanitization added in the same commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Second PR-review round on #948. All four reproduced before fixing.
1. HIGH — reviewed HTML executed in the review page. BlockTagger re-emitted
attributes verbatim, escaping values but never filtering attribute names or
URL schemes. The Markdown path has had _safe_href scheme-allowlisting all
along; the HTML path had nothing. Reproduced: a draft.html containing
`<img src=x onerror=...>`, `<a href="javascript:alert(1)">` and an <iframe>
passed straight into the page a reviewer opens — and reviewing a landing-page
draft is a documented use of this skill.
sanitize_attrs() drops on* handlers, srcdoc and srcset, and runs href/src/
action/formaction/poster/cite/background through the scheme allowlist.
_safe_href now strips control characters before reading the scheme (so
`java\tscript:` cannot smuggle one) and allows data:image only for image
attributes. DROP_TAGS removes iframe/object/embed/frame/base/applet as well
as script/style/head/link/meta. Verified: handlers, javascript: (plain and
tab-smuggled), and iframes all gone; https links and relative images kept.
2. MEDIUM — verify_quotes compared rendered text against raw markup. A quote
comes from window.getSelection(), which is what the browser rendered, so
selecting a sentence containing **bold**, `code` or a link never matched the
raw source. G7 had just made that blocking, so this refused legitimate
closes. Now matched against raw OR a rendered-text projection (inline markup
stripped for Markdown, tags stripped and entities unescaped for HTML). A
fabricated quote is still caught — verified both directions.
3. MEDIUM — state["waiver"] was never cleared, so after waived-close → reopen →
a clean round, close still printed the old waiver reason. For a tool whose
premise is an honest record of what was actually reviewed and waived, that is
its own integrity bug. Cleared whenever a close passes with zero refusals.
4. LOW — status returned 4 for both "no sidecar yet" and "collected, blockers
open". The blocked case now returns 2, matching close, so an agent can branch
on the exit code alone: 0 clear, 2 blocked, 3 collect, 4 nothing yet.
Also corrected an over-broad claim of my own: the page makes no network request
of its own, but a reviewed HTML artifact's own https: assets do load, as they
must for the review to be faithful. README and SKILL.md now say that precisely.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Both from PR review on #948, both reproduced before fixing.
G7 — a real hole in the skill's core promise. feedback_parser downgrades an
unrecognised severity heading to NIT and records it only as advisory prose in
`problems`, which cmd_close never read. Reproduced: a sidecar with `## BLOKCER`
carrying "No source. Do not ship this." collected as a NIT, and close exited 0 —
a reviewer's genuine blocker lost to a typo. Same gap covered EDIT items with no
`+ after:` line and quotes that do not appear in the target file.
close now refuses (exit 2) while the last collected round carries unresolved
integrity problems. Problems that already have their own rule are filtered via
GATED_ELSEWHERE so G2/G3 are not double-reported. Verified: typo'd severity,
missing EDIT replacement, and quote-not-in-file each refuse; a clean sidecar
still passes; a missing reviewer still reports G3 alone.
Stray artifact — quarterly-plan.review.html was committed at the repo root. It
came from a `review_page_builder.py --sample` run during the post-merge
verification sweep with cwd at the repo root, then got swept up by `git add -A`.
Removed, and .gitignore now covers `*.review.html` + `.human-gate/` so neither
this repo nor a user of the skill re-commits a disposable review page. The
sidecar (<artifact>.review.md) is deliberately NOT ignored — that is the
reviewer's feedback and belongs in git.
G7 documented in the script docstring, SKILL.md, README, the command, plugin.json
(description + derivation_note) and CHANGELOG. Not acted on: the reviewer's note
that cmd_status's success line is terse — they flagged it as "not a real issue"
and the JSON branch already carries blocking_open.
Re-verified: derive_counters --check passes, check_plugin_json --all 90/90,
write-a-skill checklist 6/6 PASS, description validator PASS, all three scripts
--help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Audits petergyang/human-review and ships a conceptual derivation that fits this
repo's stdlib-only conventions.
Audit (audit/human-review-2026-08/AUDIT.md): upstream is a well-engineered ~5,200
LOC Node app — its own test suite passes 90/90, and its security model (loopback
bind, DNS-rebinding Host check, constant-time token compare, realpath traversal
guard, inert Markdown renderer, 45-min idle shutdown) is better than most
local-server tools. It still does not fit: Node 20 + an npm runtime dependency
fails the same stdlib-only test that kept the heavier skillopt package out in
v2.11.2. Seven findings, three material — F1 (HIGH) unpinned `npx -y` executes a
newly published version on every run; F2 (MED) "do not end your turn" plus
re-poll on timeout with no headless guard or retry cap; F3 (MED) only /api/* is
token-gated.
Also: despite the name it is not a humanizer. This is human approval, not human
voice — no overlap with behuman or content-humanizer.
New plugin engineering/human-gate, three stdlib scripts, no server or socket:
- review_page_builder.py — Markdown/HTML to a single-file anchored review page
with zero network requests (~11 KB, opens over file://). Escapes before
applying inline markup, scheme-allowlists hrefs, drops script/style on HTML
input.
- feedback_parser.py — sidecar to batch.v1 JSON. BLOCKER/MAJOR/MINOR/NIT
(matching md-review) plus EDIT/NOTE/APPROVE. Verifies quotes against the real
file; strips HTML comments so a documented example cannot parse as a real
sign-off.
- human_gate.py — open/status/collect/close/reset with atomic writes and
0700/0600 state. Rules G1-G6 refuse to close on: no collected round, an open
BLOCKER/MAJOR, an unnamed reviewer, a sidecar changed after collection, an
exhausted round cap (exit 5 = escalate), or an undocumented waiver.
Loop discipline deliberately inverts upstream: no blocking poll, a headless
guard, a round cap that escalates. The sidecar is hand-writable Markdown, so the
loop closes over SSH and in CI. The optional bridge to upstream is opt-in and
always version-pinned.
Adds 3 references (7-8 sources each), a batch.v1 schema, a worked example,
cs-human-gate agent, /cs:human-gate command. SKILL.md passes the write-a-skill
6-item checklist 6/6; description validator PASS.
Counters: skills 362->363, tools 644->647, refs 741->744, agents 102->103,
commands 116->117, plugins 88->89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
