Skip to content

fix(audit): audit waivers on bulk saves and drafts; re-read grants on an edit save - #1296

Merged
rubenvdlinde merged 6 commits into
developmentfrom
fix/review-waiver-audit-followups
Nov 4, 2026
Merged

rubenvdlinde merged 6 commits into
developmentfrom
fix/review-waiver-audit-followups

Conversation

@rubenvdlinde

@rubenvdlinde rubenvdlinde commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

REVIEW, held. Follow-ups from RW129 on #1294, the stale-grants loss in the agent form, and RW150's findings on this PR.

Waiver audit

  • Bulk saves. The Agent schema sets fireEventsOnBulkWrites: true (register 0.52.0, Agent 0.13.0, app version raised for gate 120). Test reads the shipped schema.
  • Cost of that, accepted. The coordinator decided the bulk event cost is acceptable, since agents are rarely bulk-imported. RW150 estimates tens of seconds per 1,000 agents in one request, not measured: 10 to 15 statements per row, plus text-extraction jobs, metric rows and activity rows. A bulk save of agents also runs the Talk bot listener per row, as a single save does.
  • OpenRegister floor. MIN_OPENREGISTER_VERSION rises to 2.1.43-unstable.20261102171844, the first tag holding 959be02f8; an older version drops the setting. That constant is only named in a message, so BulkWriteEvents also joins the required classes, which is what the upgrade check actually tests.
  • Draft agents. AgentWaiverChangeListener also listens to DraftCreatedEvent and DraftUpdatedEvent. Leaving draft raises DraftUpdatedEvent and then ObjectCreatedEvent for the same agent; the second is not recorded again. The mark is keyed on the agent's uuid, now tested (RW150 mutant L8).
  • Tests added. Literal action token, a matcher that throws in the listener, registration of all four events.

Residual: a draft written before this ships is not recorded when it leaves draft. OpenRegister's system operations and a failed old-entity read still raise no usable event.

Talk

A new TalkSafeText decides which names go in a Talk message. It now covers ApprovalPreviewBuilder::toolLine, both approval lines in DeliveryService (the schedule, flow and skill name at line 594 included), and TalkApprovalNotifier::requestText, which had the same exposure and was not on the list. An unsafe name reads "a tool" or "an action". The id pattern now anchors with D, so a trailing newline no longer passes.

Agent form

An edit save re-reads the stored tools first, through the same store.fetchObject that seeded a routed form. Hermiq's own agent endpoint gives an instance admin a reduced row without tools; reading that as "no grants" was the wipe RW150 found. An admin can still rename the agent and its grants stay.

  • Only another tab changed the grants: the fresh value is carried.
  • This form changed them too: nothing is saved.
  • The read fails: nothing is saved.
  • The read holds no list (missing, null, string) and the form read grants: nothing is saved.

Tests cover each of these, and the same-size waiver toggle (RW129 F6).

Not closed: only tools is re-read, so other fields changed elsewhere are still overwritten from the form's copy, and a write between the re-read and the save can still be lost.

Catalogue form. RW150 asked whether edit is reachable from a reduced row. From the code, no: the catalogue is configured with register and schema and has no endpoint of its own, so its rows come from OpenRegister's objects API. AgentCatalog::page is served at /api/agents, which the form does not read. Chat mounts the form with a draft, and the detail page uses fetchObject. Read from code, not run live.

Verified

  • Failing first: the form specs failed on the old modal, and the listener specs on the old listener.
  • Capped mutants: 29 in the first round and 22 on these fixes, all killed; controls survived. H3 and C1 survived at first, got tests and were then killed.
  • One heavy run over this branch and the follow-up tests PR merged on development (tree 8749f06d, which holds refactor(oversight): split the tool-oversight controller's collaborators #1297): pcov suite --fail-on-risky exit 0, 3070 tests, 1 skip, 0 risky; vitest 0; check:specs 0; lint 0; format 0; l10n 0; gates 0, 75 of 75 applicable ran, gate 122 PASS, gate 123 PASS with 25 of 34 4-star checks. Merging development afterwards changed nothing in that tree.
  • Not run on a live instance.

Assisted by Claude Code.

🤖 Generated with Claude Code

The Agent schema sets fireEventsOnBulkWrites, so an OpenRegister bulk
save raises the object events per row (register 0.52.0, Agent 0.13.0).
The waiver listener also listens to DraftCreatedEvent and
DraftUpdatedEvent, and does not record the created event that follows
a save out of draft a second time. A tool id that Talk could read as a
mention, link or markdown is no longer put in the approval text.

Assisted-by: ClaudeCode:claude-opus-5-5
The form wrote back the grants as they were when it opened, so a change
made meanwhile in another tab was undone. The save now reads the stored
value first and carries it. When it moved and the form changed the
grants too, or it cannot be read, nothing is saved.

Assisted-by: ClaudeCode:claude-opus-5-5
…form

Hermiq's agent endpoint gives an admin a row without tools, which the save
read as no grants and wrote back as an empty list. The save now reads through
store.fetchObject, and refuses when that read holds no list and the form read
grants. OpenRegister's floor rises to the first tag reading
fireEventsOnBulkWrites. Every approval line to Talk goes through TalkSafeText.

Assisted-by: ClaudeCode:claude-opus-5-5
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ 52486c4

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
check-manifest ✅
test-l10n ✅
format ❌
check-l10n-js ✅
check-schema-l10n ✅
check-bundle-budget ✅
composer ✅ ✅ 126/126
npm ✅ ✅ 854/854
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ❌ 3042/3049
Newman ✅ 15/15
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-11-04 02:40 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ c97abe1

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
check-manifest ✅
test-l10n ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
check-bundle-budget ✅
composer ✅ ✅ 126/126
npm ✅ ✅ 854/854
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ❌ 3056/3063
Newman ✅ 15/15
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-11-04 07:27 UTC

Download the full PDF report from the workflow artifacts.

@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ 82181d9

Check PHP Vue Security License Tests
lint ✅
phpcs ✅
phpmd ✅
psalm ✅
phpstan ✅
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
check-specs ✅
check-manifest ✅
test-l10n ✅
format ✅
check-l10n-js ✅
check-schema-l10n ✅
check-bundle-budget ✅
composer ✅ ✅ 126/126
npm ✅ ✅ 854/854
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ❌ 3063/3070
Newman ✅ 15/15
Playwright ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into development.
Hydra gates ✅

Quality workflow — 2026-11-04 10:48 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 50f4537 into development Nov 4, 2026
50 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant