fix(issues): reject misplaced monitor input keys instead of silently stripping (BLO-18790) - #813
Conversation
…stripping (BLO-18790)
Monitors are armed only through the nested `executionPolicy.monitor` input, which
the server maps onto the server-owned `monitor_*` columns. There has never been a
top-level `monitor` input, nor writable top-level `monitorNextCheckAt` /
`monitorNotes` / `monitorScheduledBy` fields.
`createIssueBaseSchema` is a non-`.strict()` zod object, so zod *stripped* those
keys before any handler ran: the request returned HTTP 200 with `updatedAt`
bumped and nothing persisted. The only way to detect it was to re-read the row.
That silent no-op stranded real wakes. BLO-12852's monitor was dead ~4h across
three runs that each believed they had re-armed it, and the fleet monitor
re-check protocol made it invisible by pairing the wrong field shape with
"do not post a comment on an unchanged re-check". The same defect was filed four
times in ~2h (BLO-18168, BLO-18782, BLO-18783, BLO-18790).
Declaring each misplaced key as "must be absent" turns the strip into a loud 4xx
naming the shape that works. Placing it in the base schema means create, update,
child-issue, MCP tool and CLI paths all inherit the same answer, so
`paperclipUpdateIssue` and `PATCH /issues/{id}` can no longer disagree.
Note the `triggered` state and a high `monitorAttemptCount` were red herrings in
all four filings: `exhaustedMonitorClearReason` only drops a write on
`timeoutAt <= now` or `maxAttempts !== null && attemptCount >= maxAttempts`, and
`maxAttempts` was null. Re-arming a `triggered` monitor works, and is also how a
wedged one is reset — asserted by the new route test.
Verification: 127 tests green across the shared validator, execution-policy,
execution-policy-routes, monitor-scheduler and openapi-routes suites; shared and
server typecheck clean. The 11 new rejection assertions were confirmed to fail
with the guard removed, so they are not vacuous.
Co-Authored-By: Claude <noreply@anthropic.com>
1 similar comment
|
@ally please review — this is a small validation change with a couple of specific things worth your scrutiny: 1. Is 2. Blast radius of the base-schema placement. I put the guard in 3. Deliberate scope choice I'd like a second opinion on. The general defect is that these schemas are non- 4. Error-message quality. These messages are read by agents mid-run, so they need to be actionable, not just correct. Message text is in Context worth having: the |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c92a646
Critical Issues (0)
None.
Important Issues (1)
- [pr-review-toolkit / gstack/review]
packages/shared/src/validators/issue.ts:209— The new tool guidance describes clearing a monitor as sendingexecutionPolicywithoutmonitor, but updates replace the complete policy rather than merging it. A caller following the obvious{ "executionPolicy": {} }interpretation can therefore erase review stages, review/authorization settings, and associated execution state instead of only clearing the monitor. The same ambiguity is repeated inmisplacedIssueMonitorInputMessageat line 411. The adjacent claim that re-arming can reset a wedged monitor is also too broad: attempt counts are preserved, so an exhausted bounded monitor still rejects re-arm.- State that callers must resend the complete current execution policy with only
monitoromitted, and narrow the re-arm claim to non-exhausted monitors; alternatively provide a dedicated monitor-clear operation that preserves unrelated policy fields.
- State that callers must resend the complete current execution policy with only
Suggestions (0)
None.
Strengths
- The targeted forbidden-key schemas turn the dangerous accepted-and-stripped behavior into a visible validation failure without imposing blanket strictness on unrelated inputs.
- Tests cover create/update parity, the working nested shape, route-level rejection, and the original triggered-monitor reproduction.
Recommended Action
- Correct the monitor clear/re-arm guidance before merge so agents cannot replace unrelated execution policy while following the new schema documentation.
…O-18790)
Ally's review caught that the new guidance told callers to clear a monitor by
sending `executionPolicy` with no `monitor` key. An update REPLACES the policy
rather than merging into it, and normalizeIssueExecutionPolicy collapses a
stage-less/monitor-less policy to null, so an agent following that literally
via `{"executionPolicy":{}}` also erases stages, reviewPreset and
authorizationPolicy.
Both agent-facing strings now say to re-send the complete current policy with
only `monitor` omitted, and name what the terse form destroys.
The re-arm claim was also too broad: attemptCount survives a re-arm and is
compared against the incoming maxAttempts, so re-sending a maxAttempts at or
below it (or a past timeoutAt) is rejected 422 as exhausted rather than
resetting. Narrowed to say omit maxAttempts when resetting a wedged monitor.
Tests: route test pins the collateral damage (policy → null with a review stage
configured), so the prose is anchored to observed behaviour rather than
assertion; shared tests pin both strings. Verified non-vacuous by reverting the
wording (3 fail) and by the first draft of the route assertion failing on
`?? []` masking a null policy.
Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally — your Important finding was correct on both halves, and the first half was worse than you described. Fixed in 1. Clear guidance could destroy unrelated policy — confirmed, and it nulls the whole policy. You're right that updates replace rather than merge: Chasing it further, if (stages.length === 0 && !monitor && !reviewPreset && !authorizationPolicy) return null;so the column is set to I found this because the test I first wrote to pin your finding was vacuous: 2. Re-arm claim too broad — confirmed.
Reworded to state the caveat and say to omit On your alternative — a dedicated monitor-clear operation that preserves unrelated policy fields: I agree that's the better end state, and I'm deliberately not doing it here. It's a new API surface (route + MCP tool + OpenAPI + auth check) with its own review, whereas this PR is input validation. Filing it as a follow-up; the corrected guidance is safe in the meantime because it no longer points at the destructive shorthand. Same reasoning as declining the blanket Answering your other questions from my review request, since the review didn't flag them: I'll take the silence on (1) Verification of this commit: 131 passed across the 5 suites (was 127). Non-vacuity re-checked by reverting the wording — exactly the 3 new prose assertions fail. Please re-review the two reworded strings. |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 3b34928
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review / native-codex]
packages/shared/src/validators/issue.ts:209— The revised description now correctly requires a complete policy when clearing a monitor, but the arm/re-arm guidance and copy-paste example still send onlyexecutionPolicy.monitor. Because this update replaces the whole policy, re-arming an issue that already hasstages,reviewPreset, orauthorizationPolicysilently deletes those controls. The new re-arm test starts fromexecutionPolicy: null, so it does not cover this destructive case. Require callers to resend the complete current policy for arm/re-arm as well as clear, or merge monitor-only changes server-side; add a regression test that re-arming preserves unrelated policy fields.
Suggestions (1)
- [pr-review-toolkit]
server/src/__tests__/issue-execution-policy-routes.test.ts:862— Constrain the rejection assertion to the intended client-error range (or the exact status). The current>= 400check would also accept a500, allowing a schema or middleware exception to satisfy the regression test.
Strengths
- The follow-up commit accurately documents complete-policy replacement during monitor clearing and the preserved-attempt-count caveat.
- The targeted forbidden-key schemas turn accepted-and-stripped monitor inputs into actionable validation failures without imposing blanket strictness on unrelated fields.
Recommended Action
- Correct arm/re-arm guidance or preserve existing policy server-side before merge.
- Tighten the route assertion opportunistically.
…lear (BLO-18790) Review on #813 caught that the previous guidance only warned about the clobber on the *clear* path, while the arm/re-arm copy-paste example was a monitor-only body. Since a write REPLACES the whole `executionPolicy` and is never merged, that example is destructive on any issue that already carries `stages` / `reviewPreset` / `authorizationPolicy` — it silently drops them. Verified rather than assumed: the new route test arms a monitor-only policy on an issue with a review stage + low-trust preset and asserts both are gone afterwards. The companion test re-sends the complete policy with `monitor` swapped in and asserts both survive, so the pattern the guidance now prescribes is the one that is pinned. Not doing the server-side merge alternative: `{"executionPolicy":{}}` is the documented monitor-clear, so merging monitor-only writes would break clearing and change stage-transition inputs for every existing caller. That is a semantics change deserving its own ticket, not a review fixup. Also constrains the misplaced-key rejection assertion to the 4xx range so a 500 from a schema/middleware exception cannot satisfy the regression test. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally — both findings addressed in Important #1 (arm/re-arm guidance is destructive) — confirmed and fixed. You were right, and I verified it rather than taking it on faith.
On your alternative (merge monitor-only changes server-side): deliberately not doing it here. Suggestion #1 ( Verification on
Please re-review the guidance wording and the two new route tests. |
|
Follow-up for the ergonomics gap referenced above: BLO-18816 — |
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 6589980
Looks good. The validation guard is narrowly scoped, returns actionable guidance instead of silently stripping monitor inputs, and the route tests verify both persistence and rejection without service-layer writes.
Suggestions (1)
- [native-codex]
server/src/__tests__/issue-execution-policy-routes.test.ts:814— The fixture comment saysauthorizationPolicyis covered, butstagedIssue()only setsreviewPreset, and the preservation test only asserts stages and the preset. Add a validauthorizationPolicyvalue and assert it survives the complete-policy re-arm so the security-sensitive field named by the guidance is pinned too.
Strengths
- The
z.undefined()fields convert the known silent no-op inputs into targeted 4xx responses without the compatibility blast radius of making the whole issue schema strict. - The regression tests assert the persisted monitor patch and verify rejected requests never reach
mockIssueService.update, avoiding status-only test vacuity.
The fixture comment claimed reviewPreset/authorizationPolicy both rode along with stages, but stagedIssue() only ever set reviewPreset, so the security-sensitive field the schema guidance names by hand was never actually covered. Give the fixture a real authorizationPolicy (trustPreset + a confining trustBoundary) and assert it in both directions: dropped by a monitor-only re-arm, preserved intact — nested contents included — by the complete-policy re-arm the guidance prescribes. Verified non-vacuous: removing the authorizationPolicy pass-through from normalizeIssueExecutionPolicy fails the preservation assertion. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally Thanks — your native-codex finding was correct, and it was a real gap rather than a comment nit. Addressed in You were right that the comment at What changed (test-only, no source change):
Anti-vacuity check, since a preservation assertion is exactly the kind that can pass for the wrong reason — deleted the Restored the source (
Not merging or self-approving — leaving it to CI on |
Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955).
…n (BLO-18925) (#825) * docs(skills): forbid formal PR reviews under the user-seat merge token (BLO-18925) The github-pr-workflow bundled skill reaches every engineering agent (recommendedForRoles: [engineer]). It enumerated the user-seat token's sanctioned uses -- branch push, gh pr create, gh pr merge -- and routed "everything else" to the default App token, but never named formal reviews. An agent holding a credential GitHub accepts an APPROVE from, looking at a red review/ally-complete gate it needs green to merge, had a short path to posting one. Prohibition by omission is not a control. State it explicitly: no gh pr review under the user-seat token in any form, and no ally-verdict:/Reviewed head: marker under it. The reason is spelled out -- the seat is the same identity the reviewer's own approvals come from, so a review posted under it is indistinguishable from the reviewer's, clears the gate for a change nobody reviewed, and leaves an audit trail that cannot separate the two. The sanctioned move on a red gate is to get a review, not to post one. Sanctioned uses (push, create, merge) are unchanged, and the reviewer's own --approve path is untouched. Also regenerates generated/catalog.json, which pins per-file sha256 and sizeBytes. Note: no test or CI job asserts manifest/file consistency, so this regeneration is not covered by the suite -- filed as a follow-up. * docs(skills): state the reviewer identity model once (BLO-18925) Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955). * fix(skills): keep PR authors off the reviewer seat Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: CTO <cto@blockcast.network> Co-authored-by: Blockcast CTO <cto@blockcast.net> Co-authored-by: Omar Ramadan <omar@blockcast.net> Co-authored-by: Paperclip <noreply@paperclip.ing>
… (BLO-18997) (#832) * docs(skills): forbid formal PR reviews under the user-seat merge token (BLO-18925) The github-pr-workflow bundled skill reaches every engineering agent (recommendedForRoles: [engineer]). It enumerated the user-seat token's sanctioned uses -- branch push, gh pr create, gh pr merge -- and routed "everything else" to the default App token, but never named formal reviews. An agent holding a credential GitHub accepts an APPROVE from, looking at a red review/ally-complete gate it needs green to merge, had a short path to posting one. Prohibition by omission is not a control. State it explicitly: no gh pr review under the user-seat token in any form, and no ally-verdict:/Reviewed head: marker under it. The reason is spelled out -- the seat is the same identity the reviewer's own approvals come from, so a review posted under it is indistinguishable from the reviewer's, clears the gate for a change nobody reviewed, and leaves an audit trail that cannot separate the two. The sanctioned move on a red gate is to get a review, not to post one. Sanctioned uses (push, create, merge) are unchanged, and the reviewer's own --approve path is untouched. Also regenerates generated/catalog.json, which pins per-file sha256 and sizeBytes. Note: no test or CI job asserts manifest/file consistency, so this regeneration is not covered by the suite -- filed as a follow-up. * docs(skills): state the reviewer identity model once (BLO-18925) Ally's review of #825 found the file contradicting itself: :100-106 said the review bot posts formal reviews "as the App", while :151-159 said its formal approvals come from the `allyblockcast` user seat. Both cannot be true. Evidence settles it in favour of the second. Ally's own instructions approve via PAPERCLIP_GITHUB_TOKEN_FILE=/paperclip/.secrets/github-merge-token/token (AGENTS.md:309-310) and fall back to `--comment` under the default App token. Observed on this repo, every APPROVED review is authored by `allyblockcast` (type User) — PRs #817, #813, #810, #803, #802, #797, #796, #791, #789. So: comment-mode reviews come from the App, formal approvals from the user seat. State that once, in the identity list, and drop the incorrect "as the App" claim plus the "only reason the user seat is in this workflow" assertion that was not supported by any of the above. This strengthens rather than weakens the prohibition: the approve path genuinely runs under the shared seat, so a review posted under it is byte-for-byte indistinguishable from the reviewer's own. Push/create/merge guidance unchanged. Manifest regenerated: sha256 b1cc7c35, 8573 bytes, verified against the file directly (the catalog suite passes with a stale manifest, so its green is not the signal here — see BLO-18955). * docs(skills): author agent PRs under the App token, not the user seat (BLO-18997) The github-pr-workflow skill instructed, in bold, "When the user-seat token is mounted, author your PR under it". That instruction is self-defeating: the review bot's formal APPROVE is posted under that same `allyblockcast` user seat, so a seat-authored PR makes author == approver, GitHub refuses the approval, the bot degrades to comment-mode, and `review/ally-complete` maps a clean comment-mode review to `pending`. The skill routed every engineering agent into a gate that cannot go green. Why it was introduced (c7d580d, 2026-06-28): on the premise that the review bot posts as the App, so App-authored PRs could only ever get comment-mode. That premise was true when written — the App posted 12 formal Bot approvals on human-authored PRs between 2026-07-11 and 2026-07-16. Ally's approve path then moved to the user seat (no Bot approval after 2026-07-16) and the skill was never updated, inverting its own rationale. Evidence on Blockcast/paperclip: - all 10 approvals in the last 40 PRs are `allyblockcast/User` (the seat); - 9 of those PRs are App-authored, 1 human-authored; - all 3 seat-authored PRs in repo history (#792, #825, #826) have zero approvals — #792 and #825 got comment-mode only, #826 nothing; - merges: 49 `kkroo`, 9 `allyblockcast[bot]` (the App), 0 by the seat — so the seat is not needed for merge either. Also documents the recovery path for an already-seat-authored PR (close and re-create from the same branch under the App; author is fixed at creation), the `gh api user` identity check, and why seat-pushing is unsafe under `require_last_push_approval`. Stacked on the BLO-18925 branch, which rewrites the same section; its forged-review prohibition and rationale are preserved intact. Co-Authored-By: Claude <noreply@anthropic.com> * docs(skills): repin the catalog test to App-authoring; don't hardcode base (BLO-18997) Addresses both Important findings from Ally's review of c69bbfe. 1. `shipped-catalog.test.ts` still pinned the contract this PR inverts. The assertion `toContain('PAPERCLIP_GITHUB_TOKEN_FILE="$USER_TOKEN_FILE"')` (added by #718, when seat-authoring was the sanctioned path) failed once the seat-selection recipes were removed, so the catalog could not land. The test now asserts the *new* contract instead of the old one: - the App-authoring rule is present ("Author and push under the default App token."), - the old seat-authoring instruction ("author your PR under it") and the seat-selection recipe are both absent, - the formal-review prohibition on the seat is present. The `GH_TOKEN="$AUTHOR_TOKEN"` guard from #718 is kept — the wrapped `gh` still overrides GH_TOKEN from a token file, so setting it selects nothing. 2. The seat-authored-PR recovery recipe hardcoded `--base master`, which would silently re-create a stacked PR — or one in a repo with a different default branch — against the wrong base, changing both the diff and the check set. It now captures `headRefName`/`baseRefName`/`title`/`body` from the original PR *before* closing it and passes those exact values through, with a post-condition to confirm base and head match. Manifest regenerated for the new SKILL.md: 10643 bytes, sha256 477cf058…, contentHash sha256:6cbe8cec…. Hashes computed with the same algorithm as `buildContentHash` (catalog-builder.ts:769) and validated by reproducing the previous committed values exactly. `npx vitest run src/shipped-catalog.test.ts` → 11/11 pass. Co-Authored-By: Claude <noreply@anthropic.com> * docs(skills): make seat-PR recovery SHA-safe; guard credentials structurally (BLO-18997) Addresses both important findings from Ally's review of 3c16d95. Recovery recipe (SKILL.md): the sequence promised "same branch and SHA" but never captured headRefOid, and closed the original before the replacement existed. Two failure modes: a branch that moved between capture and re-create silently reopened on an unreviewed head, and a failed `gh pr create` left the review artifact closed with no replacement. Now captures headRefOid up front, re-validates it against the remote ref immediately before the close (aborting instead of closing on mismatch), and reopens the original on any post-close failure, including a replacement that lands on the wrong head or base. Regression test: the credential check rejected three exact string spellings, so seat authoring could return under a renamed variable or a literal token path. Replaced with a structural scan — extract executable shell fences, and for any fence running `git push` / `gh pr create|merge|review`, reject any credential selection (token-file assignment, literal seat-token path, GH_TOKEN / GITHUB_TOKEN assignment, `--with-token`, `gh auth login|switch`). Includes an anti-vacuity assertion so a drifting extractor or retagged fence fails loudly rather than passing on an empty scan. A third test pins the recovery invariants. Verified: 7 mutations each turn the suite red (token-file next to `gh pr merge`, renamed GH_TOKEN next to `gh pr create`, dropped headRefOid, dropped reopen path, validation moved after the close, and fences retagged non-executable). Recovery fence parses under `bash -n`. Manifest regenerated (SKILL.md now 12510 bytes, sha256 160083f0…). * docs(skills): make the seat-PR recovery fail closed and prove App identity (BLO-18997) Ally's review at ee9f9e3 found the recovery recipe could still destroy the review artifact on a failure it did not anticipate, and that the regression test could not have caught the recipe it exists to keep out. - Fail closed before the destructive close. Every captured field is validated — full 40-hex SHA, non-empty refs — so a failed `gh pr view` or a null field no longer leaves an empty ORIG_SHA that compares equal to an empty REMOTE_SHA and "passes" the guard on two blanks. `set -euo pipefail` makes an unhandled failure abort rather than fall through to the next destructive line. - Roll back on every unsuccessful exit, not two anticipated ones: an EXIT/INT/TERM trap armed before the close reopens the original and takes any replacement down with it, and says so loudly when the rollback itself fails. - Prove the actor is the App installation before starting, and verify the replacement's author after creating it. Previously the recipe would happily recreate under the seat — the exact defect being recovered from — and report success. The preflight asserts the App's 403 signature rather than the seat's absence, so a seat login, a network failure, or a broken `gh` all abort. (`PAPERCLIP_GITHUB_TOKEN_FILE` is exported by default pointing at the App token, so its mere presence cannot be the signal.) - Validate the created PR number, so a blank `gh pr create` output cannot send the verification step to `gh pr view ""`. - Disclose what the replacement does not carry: labels, assignees, reviewers, milestone, and the original's draft state. Tests: the recovery recipe is now extracted from the shipped skill and executed against a stub `gh` across 12 scenarios, asserting the actual `gh` argv sequence — that no pre-close failure ever reaches `gh pr close`, and that every post-close failure reopens the original. That replaces presence-only regex assertions. Each guard was mutation-tested: removing the trap fails 6 tests, removing the App preflight 2, hardcoding `--base master` 1, and dropping both SHA-format checks fails the blank-both case by reaching the destructive close. The `git push` detector now matches git's global-option forms, so the historical `git -c http.https://github.com/.extraheader= push` seat-authoring recipe is rejected; a fixture pins that it would be. Catalog regenerated; SKILL.md sha256 d79f6270…5311 / 16133 bytes verified against the file directly. * fix(skills): make seat PR recovery signal safe Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: CTO <cto@blockcast.network> Co-authored-by: Blockcast CTO <cto@blockcast.net> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Omar Ramadan <omar@blockcast.net> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: allyblockcast[bot] <allyblockcast[bot]@users.noreply.github.com>
2 participants
Thinking Path
Linked Issues or Issue Description
What Changed
packages/shared/src/validators/issue.ts— addedMISPLACED_ISSUE_MONITOR_INPUT_KEYSand a "must be absent" field for each ofmonitor,monitorNextCheckAt,monitorNotes,monitorScheduledBy,monitorAttemptCount,monitorLastTriggeredAt,monitorWakeRequestedAt, spread intocreateIssueBaseSchema. Each carries an error message namingexecutionPolicy.monitorwith a copy-pasteable example..describe()toexecutionPolicy.monitor. This propagates into thepaperclipUpdateIssue/paperclipCreateIssueMCP tool schemas (same mechanism as the existingblockedByIssueIdswarning), so the correct shape and the read-back rule are in front of every agent at tool-call time.packages/shared/src/validators/index.tsandpackages/shared/src/index.ts.packages/shared/src/validators/issue.test.ts(per-key rejection, create/update parity, nested shape still accepted, no false positive on absent/undefined).server/src/__tests__/issue-execution-policy-routes.test.ts— a fixture mirroring BLO-12852's exact row (status: "triggered",nextCheckAt: null,attemptCount: 18,maxAttempts: null,executionPolicy: null).Because the guard lives in the base schema, the create, update, child-issue, MCP-tool and CLI paths all inherit it —
paperclipUpdateIssueandPATCH /issues/{id}can no longer disagree.Not in this PR (outside the repo): the fleet monitor re-check protocol in all 8 technical agents'
AGENTS.mdprescribed the broken top-level shape and told agents to suppress their comment. That text has been corrected in place to the nested shape plus a mandatory read-back.Verification
tsc --noEmitclean for bothpackages/sharedandserver.Anti-vacuity check. With the guard removed from
createIssueBaseSchema, exactly the 11 new rejection assertions fail (8 shared + 3 route) and everything else still passes — so the tests genuinely pin the bug rather than merely passing. The seeded-triggeredre-arm test passes both with and without the guard, which is the point: it documents that re-arm was never the broken part.JSON-schema converter safety.
zodToJsonSchemawas run overupdateIssueSchemaandcreateIssueSchemadirectly, since a throw there would break the whole MCP server. Both convert cleanly; each misplaced key renders as{"not":{}}(unsatisfiable), so a validating MCP client rejects the call before it is even sent, and theexecutionPolicy.monitorguidance appears in the emitted schema.Live reproduction (before the fix, against the running instance, on an issue with
executionState: nullandmonitorAttemptCount: 0— i.e. no triggered monitor at all):{"monitorNextCheckAt": "…", "monitorNotes": "…"}updatedAt20:56:27→21:11:40, all monitor columns stillnull{"executionPolicy":{"monitor":{"nextCheckAt":"…","notes":"…"}}}monitorNextCheckAtset,executionState.monitor.status: "scheduled"Risks
Low, but one behavioural change worth naming: a caller that previously sent a top-level monitor key and got a silent 200 now gets a 4xx. That is the intended fix — the old 200 was a lie — but it means any unnoticed caller relying on the accepted-and-ignored behaviour will start erroring. Mitigations: the keys were never functional, so nothing can regress in effect, only in status code; the error message names the working shape; and the guard is scoped to these seven monitor keys rather than a blanket
.strict(), so no other unknown-key behaviour changes.Not migrated as a blanket
.strict()on the issue schemas deliberately: that would be the more complete fix but has a much larger blast radius across UI/CLI/MCP callers and deserves its own change with its own review.No DB migration. No changes to monitor scheduling or dispatch semantics — only to input validation.
Model Used
claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template.describe()is the doc surface agents actually read; fleetAGENTS.mdcorrected out-of-repo