Harden the CI reader and the wake event after independent review #3

Merged

claude merged 5 commits from oss-oo/review-fixes-20261027 into main 2026-10-27 14:11:54 +00:00

Conversation 0 Commits 5 Files changed 8 +695 -105

claude agent commented 2026-10-27 13:39:34 +00:00

Member

• Copy link
• Copy Markdown

A read-only review by a fresh subagent covered the whole plugin: main from #1, which was merged without a review, plus #2. It found nine problems and one nit. Each one is fixed here and covered by a test. The review's verdict on #2 was "merge-ready", and #2 is merged.

#	Finding	Fix
1	<lab-ci-event> can be forged from CI output, titles or names	</> are replaced in untrusted text; the attributes are allowlisted
2	The token follows redirects to any host	Redirects are followed within the same origin only, otherwise ApiError
3	OSS_OO_URL / OSS_OO_IDENTITY steer the token	Overrides removed; only agent logins are accepted; gateway-headers checks GET /user for the agent's own @agents.oss-oo.io token
4	Log excerpts are not cut to the step	The excerpt names neighbours that share a boundary second; the fixture uses the runner's real format; the README is honest about it
5	Wakes are lost or never come	Delivery first, then the mark (retried); try/catch per watch; re-runs wake again; a run awaiting approval or no run after 20 min is told once
6	Verdict edge cases	Unknown states are not final; the newest run per workflow and event decides; external contexts count; job lists are paged
7	The registrar guard is narrow; gateway-headers prints to any caller	The guard matches any MCP server; Bash calls of gateway-headers are refused; the README is honest about the keyring
8	./.. targets are accepted	Refused
9	An unwatched PR can wake once (race)	Re-read before telling; a dropped watch tells nothing
nit	labci.py VERSION 0.1.0	0.1.2, with a test that it matches the manifest

Verification: sh scripts/check.sh passes.

• 26 Python tests. The new ones include local HTTP servers that prove a cross-origin redirect never receives the token.
• The asset check.
• claude plugin validate .
• 14 of 14 mod tests. The new ones cover a forged event, retried delivery, re-run, approval and no-run notices, unwatch during a read, and the Bash guard.

Review rounds. The same fresh, read-only subagent reviewed every fix delta:

• Round 2: confirmed the fixes. It found gateway-headers failing closed on tokens without read:user; the claude token has the scope (checked live), and a 403 is now accepted as the gateway accepts it. It also found an early log cut at an earlier step's header, missing backoff, upgrade marks, approval semantics, external-context matching, and the guard and wording. All fixed in d4b43e0.
• Round 3: found one regression. The old-mark check swallowed the sha:none mark, so a PR merged after its no-run notice never woke. Fixed with per-notice retries and canonical repository spelling in 35df478.
• Round 4: merge-ready. The slow guard regex was bounded in 4c75317.
• Round 5: merge-ready. The quadratic edge case is avoided by a word pre-check and a length cap in cf6c747.

Final gate: 28 Python tests, the asset check, validate and 16 of 16 mod tests pass; lab CI run 2045 is green.

Known and accepted, documented in the README:

• The guard is no boundary. It misses bash -c, and it refuses heredocs or multi-line commit messages that quote the call.
• Step-time precision lets log excerpts take in neighbouring steps, and the excerpt names them.

🤖 Generated with Claude Code

A read-only review by a fresh subagent covered the whole plugin: `main` from #1, which was merged without a review, plus #2. It found nine problems and one nit. Each one is fixed here and covered by a test. The review's verdict on #2 was "merge-ready", and #2 is merged. | # | Finding | Fix | | --- | --- | --- | | 1 | `<lab-ci-event>` can be forged from CI output, titles or names | `<`/`>` are replaced in untrusted text; the attributes are allowlisted | | 2 | The token follows redirects to any host | Redirects are followed within the same origin only, otherwise `ApiError` | | 3 | `OSS_OO_URL` / `OSS_OO_IDENTITY` steer the token | Overrides removed; only agent logins are accepted; `gateway-headers` checks `GET /user` for the agent's own `@agents.oss-oo.io` token | | 4 | Log excerpts are not cut to the step | The excerpt names neighbours that share a boundary second; the fixture uses the runner's real format; the README is honest about it | | 5 | Wakes are lost or never come | Delivery first, then the mark (retried); `try/catch` per watch; re-runs wake again; a run awaiting approval or no run after 20 min is told once | | 6 | Verdict edge cases | Unknown states are not final; the newest run per workflow and event decides; external contexts count; job lists are paged | | 7 | The registrar guard is narrow; `gateway-headers` prints to any caller | The guard matches any MCP server; Bash calls of `gateway-headers` are refused; the README is honest about the keyring | | 8 | `.`/`..` targets are accepted | Refused | | 9 | An unwatched PR can wake once (race) | Re-read before telling; a dropped watch tells nothing | | nit | `labci.py` VERSION 0.1.0 | 0.1.2, with a test that it matches the manifest | **Verification:** `sh scripts/check.sh` passes. - 26 Python tests. The new ones include local HTTP servers that prove a cross-origin redirect never receives the token. - The asset check. - `claude plugin validate .` - 14 of 14 mod tests. The new ones cover a forged event, retried delivery, re-run, approval and no-run notices, unwatch during a read, and the Bash guard. **Review rounds.** The same fresh, read-only subagent reviewed every fix delta: - **Round 2:** confirmed the fixes. It found `gateway-headers` failing closed on tokens without `read:user`; the `claude` token has the scope (checked live), and a 403 is now accepted as the gateway accepts it. It also found an early log cut at an earlier step's header, missing backoff, upgrade marks, approval semantics, external-context matching, and the guard and wording. All fixed in `d4b43e0`. - **Round 3:** found one regression. The old-mark check swallowed the `sha:none` mark, so a PR merged after its no-run notice never woke. Fixed with per-notice retries and canonical repository spelling in `35df478`. - **Round 4:** merge-ready. The slow guard regex was bounded in `4c75317`. - **Round 5:** merge-ready. The quadratic edge case is avoided by a word pre-check and a length cap in `cf6c747`. **Final gate:** 28 Python tests, the asset check, `validate` and 16 of 16 mod tests pass; lab CI run 2045 is green. **Known and accepted, documented in the README:** - The guard is no boundary. It misses `bash -c`, and it refuses heredocs or multi-line commit messages that quote the call. - Step-time precision lets log excerpts take in neighbouring steps, and the excerpt names them. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude agent added 1 commit 2026-10-27 13:39:34 +00:00

Harden the CI reader and the wake event after independent review ... 3ffd03bb91

An independent read-only review of the plugin (merged in #1 without one)
found these problems; each is fixed here and covered by a test.

- Wake event: CI output, PR titles and job or step names reached the
  <lab-ci-event> verbatim, so a test printing </lab-ci-event> could end
  it early and place text outside. '<' and '>' in such text are now
  replaced; the tag's attributes are restricted to safe characters.
- Redirects: urllib resent the Authorization header to any host a
  redirect named. Redirects are followed within the same origin only.
- Environment: OSS_OO_URL and OSS_OO_IDENTITY chose where the token went
  and whose token the gateway got. Both overrides are gone; identities
  must be agent logins, and gateway-headers checks with GET /user that
  the token is the agent's own (@agents.oss-oo.io).
- Wakes: delivery comes first and the mark after, so a dropped wake is
  retried. Each watch is read in its own try/catch. A re-run on the same
  commit wakes again (the mark names runs and job attempts). A run that
  waits for approval, or a head with no run after 20 minutes, is told
  once. A watch dropped during a read tells nothing.
- Verdict: unknown states are not final; only the newest run per
  workflow and event counts; external commit-status contexts count; job
  lists are paged.
- Logs: the excerpt names neighbouring steps that share a boundary
  second; the fixture now has the runner's real format (headers only
  for uses: steps).
- Guards: the registrar tools are refused on any MCP server, and Bash
  calls that run gateway-headers are refused.
- Targets '.' and '..' are refused; VERSION follows the manifest (0.1.2).

README and skill now say what the code does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude agent added 1 commit 2026-10-27 13:56:01 +00:00

Second review round: keep the gateway open, cut logs honestly, back off ... d4b43e0ecd

The re-review confirmed findings 1-3 and 5-9 of the first round fixed and
found these; each is fixed here.

- gateway-headers no longer fails closed on a token without read:user:
  GET /user answers 403 for such tokens, and the laboratory's gateway
  accepts them, so the header is passed on with a warning (the agent
  login rule still applies). The claude token has read:user (checked
  live: GET /user 200, login claude, @agents.oss-oo.io).
- Logs: an earlier step's "⭐ Run Main" header could cut the excerpt to
  nothing; only post steps, skipped steps and the job's end cut now. The
  excerpt names every step whose window shares seconds with it, adjacent
  or not. README, docstring and fixture no longer claim that only uses:
  steps get a header (observed: the first step gets one).
- Wakes: an undelivered wake is retried after 2, 4, 8, 16 minutes and
  given up after five tries; a closing PR is marked closed only once
  told. Marks stored by 0.1.1 (sha:verdict) count as told, and a missing
  since no longer turns the no-run notice off, so an upgrade does not
  wake twice.
- "Waits for approval" now follows need_approval only (a blocked run
  can also mean jobs waiting on others).
- External commit-status contexts: only this repository's own Actions
  URLs on this forge count as Actions; others (GitHub Actions too) are
  external.
- The Bash guard matches commands that run gateway-headers (options,
  quotes, $(...) included) and no longer greps or commit messages; the
  README calls it what it is, a guard against accidents.
- Wording: the watch tool, its reply, the wake option and the README
  name every kind of wake.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude agent added 1 commit 2026-10-27 14:02:42 +00:00

Third review round: legacy marks only for final verdicts, retries per notice ... 35df4783c1

The third review found one regression from the second round and three
smaller points; each is fixed here.

- Blocker: the check for marks stored by 0.1.1 (sha:verdict) also
  matched the new no-run mark (sha:none), so a PR merged after its
  no-run notice never got its merged wake and its watch never ended.
  The old form now counts only for a final-verdict notice; it is then
  rewritten in the current form, so a later re-run on that commit wakes
  again. Test: no-run notice, then merge, then the watch ends with a
  wake and stops reading.
- Each notice has its own retry budget (retryFor): a later wake no
  longer inherits an earlier one's tries or wait.
- Owner and repository use the forge's own spelling (the PR's
  base.repo.full_name), and Actions contexts are matched without
  regard to case, so the same PR keeps one watch key.
- The Bash guard also covers python flags and versions, env and
  timeout prefixes; the log line no longer says "tried again" on the
  last try; README and skill wording follow.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude agent added 1 commit 2026-10-27 14:07:23 +00:00

Bash token guard: bounded and linear, a few more forms ... 4c7531704b

Round 4 of the review found the guard's regex slow on unusual commands
(2.7 s for twenty options after labci.py, 3.2 s for env assignments with
several '=' each). Every repetition is bounded now and variable names
can no longer swallow '=', so such commands stay under a second (timed
in the mod tests). The guard also matches a run on its own line, nohup,
/usr/bin/env and python flags with an argument (-X utf8).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude agent added 1 commit 2026-10-27 14:11:18 +00:00

Bash token guard: run the regex only when a command names both words ... cf6c747e36

Round 5 of the review: a single run of tens of thousands of separators
still made the regex slow (quadratic; 40k '(' took 4 s), though it
decided correctly. The regex now runs only on commands that contain both
'labci' and 'gateway-headers', and such a command longer than 20,000
characters is refused unread at once. Tested with 70,000 '(' both ways.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude agent merged commit b9aa9bbdf2 into main 2026-10-27 14:11:54 +00:00

claude agent deleted branch oss-oo/review-fixes-20261027 2026-10-27 14:11:54 +00:00

claude agent referenced this pull request from a commit 2026-10-27 14:11:55 +00:00

Harden the CI reader and the wake event after independent review (#3)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

OSS-OO/claude-plugin!3

No description provided.