feat(space): workflow hooks v2 — types + extensions/hooks package + binding storage (steps 1–4a) - #2473
feat(space): workflow hooks v2 — types + extensions/hooks package + binding storage (steps 1–4a)#2473lsm wants to merge 126 commits into
Conversation
Two-layer hook model (see docs/features/workflow-hooks-v2.md): - Layer 1 Hook/CustomHook = requiredData (input contract) + run (the rule) - Layer 2 HookBinding = placement on a workflow route (source/target/method/order) - HookContext = daemon-injected capabilities; hooks own their side effects - HookReturn = flow signal (continue/stop/retry) + optional payload/retry/result Old WorkflowHook* types remain until consumers migrate (steps 3-7).
HookContext must stay a generic capability surface — no domain fields. - Drop frozenPrUrl (PR-specific). A hook that needs the run's authoritative link reads it via readArtifacts() + an extensions-layer helper. - Genericize HookConnectorAccess: replace the vendor-named github member with a vendor-agnostic call(connectorId, op, args). Which connector/op to call is business knowledge that belongs in extensions, not the meta type. - HookDataFieldType: 'url' -> 'link' to match the dominant link convention (71 link vs 28 url in this area); field examples pr_url -> pr_link.
New workspace package @hyperneo/extensions/hooks (registered via the packages/extensions/* glob + a tsconfig project reference) holds the built-in workflow hook definitions — the business logic the daemon must not own. It depends only on @hyperneo/shared (hook-types subpath, so it stays pure-TS with no node/bun runtime deps). First ported hook: post_approval_only, moved from the daemon's validator. The run body is unchanged in intent; the v2 shape is: - requiredData declares the input contract (pr_link, reason) for prompt gen; - the run's frozenPrUrl dependency is replaced by getPrimaryLink(ctx), an extensions-layer helper that reads the run's authoritative link from artifacts — keeping PR business out of the generic HookContext type; - the return is the flow signal (continue/stop), not the old six-variant enum. Remaining hooks (pr_ready, review_posted, pr_merged, codex_review_approved) port in subsequent commits once the connector-call pattern is nailed.
Built-in hooks run in-process inside the daemon — same env, filesystem, and credentials — so ctx.connectors was never a real security boundary for them (they could bypass it trivially). The sandboxing it implied (stripped env, isolated HOME) is theater: a spawned subprocess can still read files, hit the network, or re-fetch credentials. Real sandboxing is a separate, harder problem this project does not solve. Remove HookConnectorAccess, ctx.connectors, and permittedExternalLookups. The GitHub-reading hooks will instead import a helper from extensions/hooks and call it directly — they run in-process and gh resolves its own credentials from the inherited env. The daemon's existing connector/sandbox machinery (buildHookRestrictedEnv, the connectors/ registry, external-state-validator, permitted-lookups injection) is the currently-live path and is torn out in steps 4 and 7, not here.
Add a direct GitHub helper (extensions/hooks/src/github.ts) that spawns gh in-process — built-in hooks run inside the daemon and inherit its env, so gh resolves its own credentials; no connector, no sandbox. The helper returns a discriminated GithubResult whose ok:false branch carries retryable, so a hook maps a rate-limit to flow:'retry' in one line via githubFailureToFlow. Port pr_merged (mark_complete merge gate): reads the run's reviewed PR via getPrimaryLink, checks state via ghGetPr — MERGED -> continue, OPEN -> retry (merge in flight), else stop; rate-limit -> retry. requiredData is empty (no agent-supplied input; identity comes from artifacts). Re-enables types:["bun"] now that the package spawns gh, and registers the workspace in bun.lock.
Completes the built-in hook registry (5/5). All three call GitHub via the in-process gh helper (no connector, no sandbox): - pr_ready (coder->reviewer): OPEN + MERGEABLE + clean-ish mergeState + no unresolved review threads -> continue; UNKNOWN -> retry; else stop. On success stamps the run's authoritative PR link as a link/pr artifact so downstream hooks bind to it via getPrimaryLink. Drops the retired validator's post-approval exemption (that traffic now routes via post_approval_only). - review_posted (review->coding): a formal review since run start, or — on an own PR — a comment. Uses ctx.runStartedAt for the freshness window. - codex_review_approved (opt-in): a codex-bot APPROVED review on the workspace's PR -> continue; else retry. Helper additions: parsePrLink, runGhGraphql, ghGetUnresolvedReviewThreads, ghGetReviewEvidence, ghGetCodexApproval, and an action.ts data-field reader. HookContext gains runStartedAt (a generic run fact for freshness scoping). The GraphQL ops are first-page-only and faithful in intent; field details need online validation in step 7.
Step 4a: persistence for the two-layer hook model, alongside the legacy hooks field (retired in step 7). SpaceWorkflow / SpaceWorkflowInput / UpdateSpaceWorkflowParams gain hookBindings (HookBinding[]) and customHooks (CustomHook[]). Migration 191 adds hook_bindings + custom_hooks JSON columns to space_workflows (idempotent); the helper schema and repository mapping (read/insert/update) round-trip them. db-schema-parity passes. No behavior change yet — the old WorkflowHook engine still runs off the legacy hooks field. The v2 engine (4b) reads hookBindings; built-in workflows are re-seeded as bindings in 4d.
…nish-the-engine-wire-it-in-step-4b-pr
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8aa915988
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The two-layer hook model now governs MCP-action handoffs; the old engine/types/connectors are removed. Engine (4b): rewrite executeAction off HookBindings -> built-in registry (@hyperneo/extensions-hooks) or per-workflow custom script -> run -> map flow (continue delivers w/ optional payload patch, stop blocks, retry queues with engine-managed backoff). HookContext is a daemon-injected capability surface (readState/recordState, writeArtifact/readArtifacts, queueFollowUp); hooks own their side effects. v2 hook-state repo + migrations 192 (last_flow/last_reason) and 193 (drop legacy hooks column). Wire + reseed (4c/4d): node-agent-tools/task-agent-manager construct the v2 engine from hookBindings; built-in workflows re-seeded as bindings; buildRoleSection derives the handoff data contract generically from requiredData (drops the hardcoded pr_ready/review_posted special-case). Teardown (4e): delete hook-executor, built-in-validator-registry, built-in-validators/, connectors/, workflow-hook-runtime-service, gh-lookup-helpers; rewrite workflow-hook-validation for bindings/customHooks; remove WorkflowHook* types + the hooks field + DB column; retype the web (runtime hook-state banner kept; old editor removed for step 6). Also fixes blind-written GraphQL parsing in extensions/hooks surfaced in review: asRecord-on-array bugs that zeroed unresolved-thread URLs and review/comment evidence; review_posted viewer read from the wrong envelope level; codex approval now link-driven (no GH_REPO misresolution), head-bound (APPROVED review on the head SHA, or a fresh post-head-push +1 reaction), matching the codex bot in both Bot/User forms; GraphQL routes to the PR's host; transient gh failures retry. Step 6 (web editor) and step 7 (engine tests + online GraphQL validation) land in a second push.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb22921f52
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ook 191) dev's #943 (PR #2475) added migration 191 for node_executions.last_activity_at, colliding with this branch's hook_bindings migration 191. Renumber the hook migrations to 192 (hook_bindings/custom_hooks), 193 (last_flow/last_reason), 194 (drop legacy hooks column). Also adds last_activity_at to the space-agent-schema test helper to match dev's schema.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 314e11a6c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- pr_ready stamps the run's authoritative PR identity under an engine-reserved artifact key (__pr_validated__); save_artifact rejects __-prefixed keys, so a same-node agent can no longer overwrite the validated identity and swap the PR the merge gate binds to. coding-artifact-profile's merge-gate resolver prefers the validated artifact, falling back to agent-written links only before the first handoff. - HookBinding.targetNode is now optional, so non-routed methods (mark_complete, save_artifact, …) can be bound; the validator requires it only for send_message. - validateCustomHooks rejects a custom hook id that shadows a built-in (which resolveHook would otherwise silently ignore). - Custom-script retryAfterMs is clamped to [1s, 1d] so a malformed value can't spin a rapid replay loop or overflow the timer. - buildRoleSection emits the handoff data contract for each target of a multi-target channel, not just single-target channels. Deferred to step 6/7 (noted in review threads): per-binding retry-state keying, the web approval controls / node-rename caller remap (web editor, step 6), and richer script side-effect channels (the simplified-scripts design is intentional).
- runGhGraphql rejects envelopes carrying a top-level `errors` array (previously reported success on a null-data error response). - runGhGraphql validates the PR host against a trusted GitHub set (github.com, ghe.com, or the configured GH_HOST) so an attacker-controlled PR link cannot redirect `gh` and its credentials at an arbitrary server. - codex approval now honors the LATEST decisive codex review on the head (a later CHANGES_REQUESTED overrides an earlier APPROVED), and uses the PR `pushedDate` for reaction freshness instead of commit `committedDate` (a locally-created commit pushed later would otherwise let a pre-push +1 false-pass). - validateWorkflowHookBindings now requires a non-empty authorizedCallers (the engine fails closed on a binding without them, so it would be silently dead). Deferred to step 7 (online validation): GraphQL pagination for review threads / evidence, and COMMENTED-review own-PR evidence — these need real-GitHub tuning.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61c423a27a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu
Recommendation: REQUEST_CHANGES
Recommendation: REQUEST_CHANGES. The cutover is architecturally sound and the prior 27 threads were well-addressed, but I found one P1 that defeats the headline PR-swap defense and breaks the mark_complete merge gate, plus a confirmed GraphQL-injection path and a cluster of untested security-critical resolvers. With online validation explicitly deferred to step 7, the pure-function paths below are the only thing standing between this merge gate and a silent bypass — they need tests now, not later.
P1 — The __pr_validated__ artifact never yields its URL → merge gate fails closed, PR-swap defense non-functional
pr_ready stamps { artifactType:'link', artifactKey:'__pr_validated__', data:{ link } }. But the daemon's prUrlOf() only extracts a URL when artifactKey === 'pr' || data?.kind === 'pr':
if (artifactType === 'link' && (artifactKey === 'pr' || data?.kind === 'pr')) { ... }The validated artifact satisfies neither. resolveInitialPrimaryLinkUrl deliberately narrows its pool to validated-only (const pool = validated.length > 0 ? validated : all), then loops that pool calling prUrlOf — which returns '' for every __pr_validated__ row. So resolveInitialPrimaryLinkUrl returns '' whenever the only PR artifact is the validated one.
Consequences (both on the immutable path, not the freshest one):
- Merge gate:
assertPrMergedusesresolveInitialPrimaryLinkUrlwithrequirePrUrl:true.createPrMergedGatereturns{ ok:false }on empty URL →mark_completecan never complete for any run that got its PR identity from the v2pr_readyhook. - Post-approval routing:
resolveInitialPrimaryLinkUrlfeeds{{pr_url}}. Empty URL → template token unresolved, andoverridePrUrl !== resolvedPrUrlmismatch throws if an override is present.
Notably the hook layer reads the validated artifact correctly (getPrimaryLink keys on __pr_validated__), so pr_merged the hook works — it's only the daemon-side resolvers that miss the new key shape. This is exactly the kind of integration miss the deferred step-7 tests are supposed to catch, but there is no test for resolveInitialPrimaryLinkUrl at all, so nothing failed.
Fix options: have pr_ready stamp data:{ link, kind:'pr' } (so prUrlOf matches), or teach prUrlOf to treat __pr_validated__ as a PR-bearing key. Either way, add a coding-artifact-profile test covering a validated-only artifact pool.
P1 — GraphQL injection via parsePrLink output into the query string
github.ts interpolates pr.owner/pr.repo directly into GraphQL strings:
`query { repository(owner:"${pr.owner}",name:"${pr.repo}") { pullRequest(number:${pr.number}) { `parsePrLink's regex captures owner/repo with [^/]+, which places no restriction to GitHub slug characters — a " or \ survives. The link comes from send_message data.pr_link, which an agent's tool-call supplies (prompt-injectable). A crafted pr_link breaks out of the owner:"..." literal and injects arbitrary query fields, executed with the daemon's gh token. pr.number is digit-only and safe; owner/repo are not.
Fix: validate owner/repo against ^[A-Za-z0-9._-]+$ before interpolating (reject otherwise → stop), or bind them as GraphQL variables instead of string interpolation. This is a pure-function change with no online dependency.
P2 — __pr_validated__ is shadowable across nodes (uniqueness key includes node_id)
The artifact upsert conflict key is (run_id, node_id, artifact_type, artifact_key) (workflow-run-artifact-repository.ts:52). pr_ready's ctx.writeArtifact carries nodeId ?? meta.nodeId, so a __pr_validated__ written from node A does not overwrite one written from node B — they coexist. readArtifactsForCtx lists all nodes' artifacts for the run and sorts by updatedAt DESC, and getPrimaryLink returns the first match — i.e. the most recently updated. A second pr_ready validation on a different node (multi-node binding, or a re-handoff) that stamps a different PR would win, swapping the authoritative identity the immutable resolver is supposed to lock. save_artifact's __-rejection stops agents, but the engine path has no single-row guarantee.
Fix: for engine-reserved keys, write with a fixed sentinel node_id, or make the read take the earliest validated row rather than the freshest. At minimum, confirm the built-in workflows only ever bind pr_ready on a single source node (they appear to — Coding/Research only — but it should be enforced, not coincidental).
P1 — Zero tests for the security-critical resolvers / extractors that step-7 defers
The deferral covers engine tests + online GraphQL validation. But several of the deferred paths are pure functions testable with static fixtures today, and they are exactly where prior review found asRecord-on-array bugs. Shipping without regression tests invites silent reverts:
extractUnresolvedThreads/extractReviewEvidence/extractCodexApproval(github.ts) — no test file exists underpackages/extensions/hooks/. A regression to wrapping GraphQL connections inasRecord()silently zeroes every count (gates always pass). Fixtures need no API access.resolveInitialPrimaryLinkUrl/resolvePrimaryLinkUrlvalidated-artifact path (coding-artifact-profile.ts) — no test at all (this is how the P1 above went undetected).- The whole
WorkflowHookEngine.executeActionflow (continue-deliver / stop-block / retry-queue-replay, payload target-stripping, multi-binding order) — noworkflow-hook-engine.test.tsruntime coverage.
I'd require at least the pure-function extractor/resolver tests in this PR; the engine-flow and online tests can reasonably follow in step 7.
P2 — No retry ceiling; a perpetually-retrying hook loops forever
retryCount increments without bound (getRetryCount(hookId) + 1) and there is no maxRetries circuit breaker. The only thing that stops a retrying send_message is the run/task reaching a terminal status. A hook that always returns retry (abandoned PR, a logic bug, a permanent rate-limit) retries every retryAfterMs indefinitely, each cycle scheduling a new timer and growing retryCount in the DB. pr_merged returning retry on a PR left OPEN is a realistic trigger. Recommend a cap (e.g. ~10–20 attempts) that converts to a terminal stop + source-session notify.
P2 (perf) — readArtifactsForCtx loads ALL run artifacts on every hook run
artifactRepo.listByRun(runId) with no LIMIT + in-memory sort + slice(50) runs on every buildHookContext (every hook, every tool call). For long runs accumulating hundreds of artifacts this is a full scan per tool call. Add ORDER BY updated_at DESC LIMIT 50 (or filter to the link type for getPrimaryLink consumers).
Lower-severity / noted
- P3:
pendingRetryableHookActionstimers aren't.unref()'d andclearAllRetryableHookActionTimersonly runs at daemon shutdown — orphaned timers for ended runs keep the process alive until they fire (then self-cancel). Minor on localhost. - P3: Two stale JSDoc comments still reference the removed
SpaceWorkflow.hooksfield (space.ts:1974,:2524). - P3:
test.skipon the hook-node-ref restamp remap (built-in-workflows.test.ts:1794) and two zero-assertion placeholder tests inserialization.test.ts— the restamp remap is worth actually covering since a renamed node would silently deactivate bindings. - Deferred (acknowledged, acceptable): per-binding retry-state keying, web approval controls, node-rename caller remap, GraphQL pagination, COMMENTED-review own-PR evidence — these track to the step-6/7 follow-up. Fine to defer.
What's solid
Migrations 192/193/194 are idempotent and guarded; the hooks column drop is safe (in-memory snapshot model, no live consumer). Repository round-trip + export/import preserve hookBindings/customHooks and are backward-compatible with old bundles. All eight deleted modules (connectors, hook-executor, validators, restricted-env, gh-lookup-helpers) have zero dangling references. authorizedCallers authorization uses daemon-trusted HookActionMeta (not agent args) — sound. save_artifact's __-key rejection covers all deriveArtifactKey paths — not bypassable.
The engine's break/continue/retry-cooldown semantics, payload target-stripping, optimistic-locking persistence, and script-subprocess cleanup are all correct on read.
…view round 3) P1: pr_ready stamps __pr_validated__ with kind:'pr', and prUrlOf now treats __pr_validated__ as PR-bearing — previously the daemon resolvers missed the new key, so resolveInitialPrimaryLinkUrl returned '' and the mark_complete merge gate (requirePrUrl:true) failed closed on every v2 run. PR-identity readers prefer the EARLIEST validated stamp so a later same-key stamp from another node can't swap the identity. P1: parsePrLink rejects owner/repo outside the GitHub slug charset (^[A-Za-z0-9._-]+$), preventing GraphQL string injection from an agent-supplied (prompt-injectable) pr_link. P1: add fixture-based tests for the pure-function resolvers/extractors (github.ts extractUnresolvedThreads/extractReviewEvidence/extractCodexApproval/ parsePrView/parsePrLink) and coding-artifact-profile's PR-identity resolution — these are exactly where prior review found asRecord-on-array bugs; engine-flow + online tests still follow in step 7. P2: hook retries now cap at MAX_RETRY_ATTEMPTS (~24h at the 30s cadence) before converting to a terminal stop, so a perpetually-retrying hook can't loop forever. P2: readArtifactsForCtx uses listRecentByRun (SQL ORDER BY updated_at DESC LIMIT 50) instead of loading every run artifact on each hook run. Also fixes a stale JSDoc ref to the removed SpaceWorkflow.hooks field, and adds the extensions/hooks test dir to knip's ignore list.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9a49c0146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lidation order (review round 3b) - Bump the export format to v4 (the schema now carries hookBindings/customHooks); a v3 client's schema would otherwise silently strip them and import lossily. ExportedSpaceWorkflow/WorkerAgent/Bundle + ExportVersion widened to 1|2|3|4; export-format.test updated (output assertions -> 4, version-rejection -> 5). - hookBindingSchema.targetNode is optional, mirroring the runtime type/validator (non-routed methods have no target). - runGhGraphql keeps a rate-limit/transient GraphQL error envelope retryable (not terminal), matching the CLI-error path. - Export transition validation admits registered built-in hook ids (a transition may reference a built-in like pr_merged even with no route binding). - validateHooks validates customHooks before resolveHook dereferences them, so a malformed customHooks (non-array / null element) returns validation errors instead of throwing.
… pipe, validated-PR in ctx (review round 3c)
- pr_ready/review_posted accept pr_url as well as pr_link: several preserved
built-in prompts instruct the agent to send data:{ pr_url } (not pr_link),
so the first handoff always stopped with "no PR link supplied". requiredData
still declares pr_link (the v2 contract); pr_url is a read-time compat alias.
- fetchPrView validates the PR host against the trusted set before invoking
`gh pr view <url>` (gh resolves the host from the URL), closing the same
SSRF path the GraphQL helper already guards.
- readUpTo keeps draining stdout past the 1MiB cap instead of abandoning the
reader, so an oversized gh response doesn't wedge the child until the kill
timer (reachable via codex's 50 review bodies).
- readArtifactsForCtx includes every __pr_validated__ artifact in addition to
the freshest 50, so a busy run can't push the authoritative PR identity out
of the bounded window and starve downstream hooks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34a0003c38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (HyperNeo)
Model: glm-5.1 | Client: HyperNeo | Provider: z.ai
Verdict: APPROVE (own-PR → Recommendation: APPROVE). The cutover is thorough and all 6 prior-round threads + 40 total review threads are resolved; head 34a0003 is green (tsc/knip/db-parity/oxlint/biome, daemon shards, web vitest). I verified each claimed P1 fix against the code: prUrlOf now recognizes __pr_validated__ (coding-artifact-profile.ts:49), parsePrLink enforces the slug charset (github.ts:315), fetchPrView validates the host before spawning gh (github.ts:253), GraphQL host is trusted-checked (github.ts:177), the asRecord-on-array bugs are fixed (github.ts:359/418/435), the viewer is read from the right envelope level (github.ts:408), and the pipe is drained past the cap (github.ts:85-90). Security controls are sound — GraphQL injection, credential-redirect, PR-swap, and __pr_validated__ forgery are all correctly mitigated; mark_complete's merge gate runs before the status flip regardless of autonomy.
The findings below are follow-ups, not merge blockers: the two P1s are (a) the engine-orchestration test gap, which the PR description explicitly scopes to step 7, and (b) a timer leak that doesn't affect correctness. Flagging them so step 7 captures them.
P1 — Follow-up dispatch timer leaks + unhandled rejection on every fast dispatch
workflow-hook-engine.ts:1555-1562
The follow-up timeout is built as new Promise((_, reject) => setTimeout(..., 30s)) fed into Promise.race. When the dispatch resolves before 30s (the normal case — nearly every time), the setTimeout is never cleared: it fires ~30s later, rejects the already-settled timeoutPromise, and with nothing awaiting it that becomes an unhandled rejection. A run dispatching several follow-ups produces several leaked timers + rejections.
Fix: keep the timer id and clearTimeout in a .finally, or use AbortSignal.timeout(). (For step 7.)
P1 — No engine-orchestration tests (2286-line suite deleted, no replacement)
packages/daemon/tests/unit/5-space/runtime/workflow-hook-engine.test.ts (deleted)
The old engine suite (chaining, stop/retry/continue precedence, retryAfterMs backoff pre-check, payload-patch re-validation, follow-up dispatch, retry scheduling/cancellation, wrapHandlerWithHooks) was removed and nothing now exercises executeAction / wrapHandlerWithHooks. Current coverage is the pure-function extractors (good) + structural assertions that the seeded bindings exist (built-in-workflows.test.ts) — but no behavioral coverage of the engine itself. The PR description scopes engine tests to step 7; flagging so they land there. Bounded today by green CI + the integration paths, but the heart of this PR is the engine and it deserves direct coverage.
P2 — readArtifactsForCtx runs 2 SQL queries per binding, per gated action (no intra-chain caching)
workflow-hook-engine.ts:799, 825-849
buildHookContext is called inside the binding loop, and each call runs listRecentByRun(runId, 50) + listByRun(runId, {artifactType:'link'}). For an N-binding action that's 2N round-trips, each JSON-parsing up to 50 rows, with identical results across bindings in the same executeAction. Compute the artifact window once before the loop and pass it in.
P2 — __pr_validated__ is hardcoded in the generic engine (domain leak)
workflow-hook-engine.ts:838
The engine is documented as generic infra with no PR knowledge (coding-artifact-profile.ts claims to be "the ONLY place in the daemon that names coding-specific kinds"), yet readArtifactsForCtx filters on the PR-specific __pr_validated__ key. The literal is also scattered across 4 files (engine, coding-artifact-profile, primary-link, pr-ready) with no shared constant. The optimization is real (busy runs push the stamp past the 50-row window), but either include all link artifacts generically, or extract the key to a shared @hyperneo/shared constant.
P2 — Dead import of a deleted module
packages/daemon/tests/unit/5-space/agent/node-agent-tools.test.ts:39-41
Imports clearBuiltInValidatorRegistry / registerBuiltInValidator from the deleted runtime/built-in-validator-registry.ts; the names are unused in the body. (This does not break CI — Daemon Unit Tests (5-space-agent-other) passes on this head — because the import resolves at runtime without forcing the missing symbols, but it's a latent smell the next runner-behavior change could trip, and tsc/knip don't catch it since tests are excluded.) Drop the three lines.
P2 — Duplicated dataOf
packages/extensions/hooks/src/hooks/post-approval-only.ts:22-27
Byte-identical to the exported dataOf in action.ts:4; the sibling hooks (review-posted.ts, pr-ready.ts) already import it from ../action. Delete the local copy and import it.
P3 — Over-exported barrel
packages/extensions/hooks/src/index.ts
The daemon imports only BUILT_IN_HOOKS + fetchPrView externally, yet the barrel re-exports ~24 symbols (all hooks, GitHub helpers, extractors, types). Narrowing it keeps internals package-private.
P3 — retryHook RPC bypasses the engine's 3-attempt conflict retry
space-workflow-run-handlers.ts:~894
Calls hookStateRepo.update once (not engine.persistStateUpdate), so a concurrent retry-timer bumping the version makes the "retry now" button throw a version conflict. Route it through persistStateUpdate for parity with the engine path.
Clean, well-layered cutover overall. Recommend merging and rolling the P1s into the step-7 test + online-validation push.
…up (review round 4) - P1: clear the follow-up dispatch timeout when the dispatch settles first — previously the orphaned 30s timer rejected the already-settled race promise with nothing awaiting it (unhandled rejection on every fast dispatch). - P2: hoist the ctx artifact window out of the binding loop (was 2 SQL round-trips per binding per gated action; now once per action). - P2: the engine's reserved-artifact keep-alive is now generic on the `__` prefix (no PR-domain key in the engine); the __pr_validated__ literal is a shared VALIDATED_PR_ARTIFACT_KEY exported from extensions/hooks and imported by pr-ready, primary-link, and the daemon's coding-artifact-profile. - P2: drop the dead import of the deleted built-in-validator-registry from node-agent-tools.test.ts; dedupe post-approval-only's local dataOf copy into the shared action helper. - P3: the extensions/hooks barrel is narrowed to what the daemon consumes (BUILT_IN_HOOKS, fetchPrView, VALIDATED_PR_ARTIFACT_KEY) — helpers and extractors stay package-private. - P3: retryHook/approveHook RPCs (and the engine's persistStateUpdate) go through a new updateWithRetry on the repository, so a concurrent retry-timer write no longer surfaces as a version conflict on the "retry now" button. Engine-orchestration tests (executeAction chaining/retry/patch/follow-up, wrapHandlerWithHooks) land in the step-7 push per review.
…ection, pinned banners (review round 5) - P1: pr_ready no longer silently re-stamps a different PR over an existing validated identity — a re-stamp is allowed only when the previously reviewed PR is no longer OPEN (a legitimate revision); otherwise stop. Downstream gates (post_approval_only, pr_merged) now bind to a genuinely immutable identity. - P1: the codex bot login matches EXACTLY the two documented forms (chatgpt-codex-connector / chatgpt-codex-connector[bot]) — a human whose login merely contains the slug no longer satisfies the gate. - P1: importing a pre-v4 export that carries a legacy `hooks` array is rejected with an actionable message instead of silently stripping the field (which imported the workflow with no hook enforcement). Checked for standalone workflows and nested bundle workflows. - P2: validateHooks throws the custom-hook errors BEFORE resolving bindings, so a malformed customHooks (non-array / null element) surfaces validation errors instead of a TypeError; resolveHook is additionally defensive against malformed entries. - P2: visual-editor serialization seeds its valid-hookId set from a new shared BUILT_IN_HOOK_IDS contract (plus bindings + custom hooks), so opening/saving no longer drops transitions that reference an unbound built-in. - P2: use-run-hook-states sources hookBindings ONLY from the run-scoped listHookStates RPC (the run's pinned definition) — a live mid-run workflow edit can no longer hide or mislabel a blocked hook.
…al (round 84 review)
P1 clearLegacyHooks passthrough: the RPC handler strips the field from
caller payloads (it is manager-internal, set only by the verified-migration
branch after the legacy-coverage check), and the manager defense-in-depth
resets it to false before that branch — a raw
spaceWorkflow.update {id, clearLegacyHooks: true} with no hookBindings
previously skipped the coverage check entirely and stripped a
legacy-bearing workflow's gates (runs fully ungated, and m197 would drop
the column next boot making it undetectable). Pinned by a flag-strip test
asserting the legacy column survives.
P2 export partial-coverage refusal: exportWorkflow applies
legacyHookCoverage — exporting legacy [A,B] with bindings covering only
[A] previously produced a clean v4 bundle (v4 carries no hooks field)
whose import recreated a workflow gated by [A] alone, silently dropping
gate B. Refused with the missing ids listed until the migration completes.
Pinned by partial-refusal and complete-export tests.
|
Round-84 response to review 4942651914 — head 4ae71a7. P1 clearLegacyHooks RPC passthrough — fixed on both planes: the spaceWorkflow.update handler strips the field from caller payloads (destructured out alongside id/spaceId), and the manager defense-in-depth resets it to false before the coverage branch (only the verified-migration path ever sets it). A raw P2 export partial-coverage — exportWorkflow now applies legacyHookCoverage and refuses when legacy hooks lack complete v2 coverage (missing ids listed): exporting legacy [A,B] with bindings [A] previously produced a clean v4 bundle whose import dropped gate B silently. Pinned by partial-refusal + complete-export tests. P2 test gaps — the restamp convergence test now goes through updateWorkflow with a real hooks column (not raw SQL); the migration-side malformed/undecodable defers were pinned in round 82 ([null]/{} cases in migration-197 tests); the web routing_unavailable banner test is still outstanding — noted. Rounds 83-84 also fixed codex findings: empty agentSlots [] rejected (eaaa3cf), retryHook rollback restores __firstRetryAt, and sameRetryableActionOwner now reconciles replacement sessions (a respawned worker with a new session id re-arms its durable queued actions instead of orphaning them) — 5defb1e. Five P3s acknowledged in the review body — deferred at your discretion unless you'd like them picked up. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ae71a71f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
legacyHookIds returns PER-PLACEMENT entries (not a deduped set) and legacyHookCoverage counts coverage per placement: a legacy workflow may place the same validator id on two routes, and v2 currently forbids one hook id on multiple bindings — a set-based check let a single binding satisfy both placements while the migration (manager update or m197 drop) deleted both legacy definitions with only one route still gated. Such a workflow now stays fail-closed (the legacy guard keeps blocking) until multi-placement is supported or the routes are re-authored with distinct hooks. Pinned by a two-placements-one-binding refusal test asserting the legacy column survives.
…n (round 85 codex) P2 required flag: isCustomHookElement requires every decoded requiredData field's `required` to be a PRESENT boolean (mirroring validateCustomHooks) — a missing value passed shape checks but buildHookValidatedHandoffLines treats it as false, letting actions send without data the stored custom-hook contract was supposed to require. Such rows now route through the corruption marker. P2 actor-role normalization: the role-activity lookup normalizes the resolver's alternate `@role:actor-role:<slot>` form to the underlying slot before comparing NodeExecution.agentName — the raw `actor-role:<slot>` string matches no execution, so every holder read inactive and, with a shared slot and one truly active holder, inactive holders' gates could run for messages they will not receive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea7c8fbf4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…m197 (round 86 codex)
P1 unrecognizable legacy columns: the retained legacy hooks column decodes
STRICTLY — malformed JSON, a non-array value, or a shapeless nonempty
array ([{}] extracts no gate ids, so coverage would read complete and the
run would construct no engine, executing ungated) surfaces a synthetic
unsatisfiable legacy entry that keeps the run guard fail-closed until the
column is healed or deliberately cleared.
P2 migration binding validation: m197's coverage pass applies the
repository decoder's FULL binding bar — a v2 binding with the right
hookId but a missing sourceNode/method/enabled/authorizedCallers counts
as coverage for the drop yet decodes as CORRUPT in the repository,
stranding the workflow fail-closed with the recoverable legacy
definitions destroyed. Malformed bindings now defer the drop. Test
fixtures gained well-formed bindings; a shape-valid-but-malformed
deferral test pins it.
There was a problem hiding this comment.
💡 Codex Review
Notify the replacement session about terminal queued actions
When sameRetryableActionOwner() admits a newly spawned replacement session, this still schedules the replay with the retired session's action.meta. If the replay terminates, replayRetryableAction() sends its failure notice to options.meta.sessionId and then clears the durable action, so the live replacement worker never learns that its accepted handoff failed. Route the notification to ownerMeta.sessionId, or migrate the replay metadata and durable key consistently.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… identity rechecks, repository-equivalent migration validation (round 87 codex) P1 batch tokens: consumeApprovalsIfCurrentBatch validates every token INSIDE the transaction, rechecking the exact humanApprovedAt immediately before its clear — the phase-1 validation previously ran before the transaction, so a concurrent consume + newer re-approval between the phases let the tx clear the NEWER row by its current version unchecked and return 'consumed' for an approval intended for a different violation. A validation abort now rolls the whole transaction back with nothing cleared (BATCH_CONSUME_ABORT sentinel). P1 route-matching coverage: legacyHookCoverage matches each legacy PLACEMENT'S ROUTE (id + method + named target) rather than counting hook ids — a pr_ready binding on a different target or method satisfied the id-only count while the legacy gate's actual route stayed ungated (and the manager/migration could then delete the original definition). A placement that names no target constrains only id + method (the legacy record did not record routing). Pinned by wrong-target and wrong-method refusal tests. P1/P2 identity rechecks: review_posted and codex_review_approved now bind their POSITIVE decisions to the PR identity they were made about — after a positive lookup they re-read the repo-backed artifacts (refreshArtifacts, mirroring pr_merged) and retry when a concurrent pr_ready replacement swapped the run's identity mid-lookup. P2 repository-equivalent migration validation: m197's coverage pass now calls the repository decoder's own isHookBindingElement with the row's node names and declared slots (method enum, node references, caller shape, routing rule) — a binding the repository would decode as CORRUPT can no longer count as coverage for the destructive drop. Pinned by a bad-method deferral test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25f40977b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lsm
left a comment
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Z.ai)
Model: glm-5.1 | Client: HyperNeo | Provider: Z.ai
Re-inspected fresh at head 25f40977b (rounds 85–87: e709cde, 25f4097, on top of 4ae71a7 reviewed earlier). All prior threads are resolved; no unresolved threads remain on any page. CI run 31891987983 green.
Verified this round
- In-transaction token validation (
workflow-hook-state-repository.ts): validate-and-clear now runs inside one transaction with aBATCH_CONSUME_ABORTsentinel — the pre-tx validation window (consume + re-approve between phases) is closed, and the rollback test proves hook_a's clear is rolled back when hook_b's token mismatches. Correct. - Route-matching coverage (
legacy-hook-coverage.ts):legacyPlacements()extracts {id, method, target} per placement; each placement consumes one enabled binding with matching id+method+target. Per-placement counting plus route matching closes both the multi-placement hole and the wrong-route hole, consistently across manager, export, and m197 (all share the helper). The undated-route fallback (target undefined → id+method only) is the right conservative reading. - Strict legacy-column decode (
space-workflow-repository.ts:422+): malformed JSON / non-array / shapeless[{}]surfaces the synthetic__unrecognizable_legacy_hooks__entry — the run guard stays fail-closed instead of constructing no engine. Correct fail-closed direction. - Repository-equivalent m197 validation:
isHookBindingElementwith the row's own node names/slots means anything the repository would decode as CORRUPT defers the destructive drop. The per-row node reconstruction mirrorsfetchNodes(same JSON config parse, same slot set). Sound. - Identity rechecks in
review_posted/codex_review_approved: repo-backedrefreshArtifacts()re-read +samePrLinkretry on mid-lookup identity swap, mirroringpr_merged.refreshArtifactsthrows when the store is unreadable, which maps to override-ineligible stop — fail-closed. Correct. requiredpresent-boolean decoding and actor-role activity normalization (decodeRoleSlotNamestrips theactor-role:prefix; the resolver-side@role:actor-role:branch is consistent with the lookup) both check out, with tests.
Findings carried from the round-84 inspection (still present at this head)
P2 — duplicate delivery window after a replacement respawn (workflow-hook-engine.ts): sameRetryableActionOwner now matches on (task, node, agent) only, but buildRetryableActionKey still embeds meta.sessionId. A re-armed dead-session record (old key) plus the replacement agent re-issuing the same send (new key) leaves two durable records and two timers — both deliver when the gate opens. The fix correctly trades loss for potential duplication, but the duplication is undocumented in the comment and untested. Consider clearing stale-keyed durable records for the same owner on re-arm, or keying identity on args+method+node+agent. Related: the re-armed replay notifies meta.sessionId — the dead session — on terminal failure, so the replacement operator never sees the failure notice.
P2 — RPC-layer clearLegacyHooks strip untested (space-workflow-handlers.ts): the round-84 test pins the manager reset only. Deleting the handler-side destructure would pass the suite while reopening the RPC passthrough the manager reset happens to cover today.
P2 — __firstRetryAt rollback restore untested (space-workflow-run-handlers.ts): the restore-path fixture seeds no __firstRetryAt and asserts only lastFlow/retryCount; the new line would pass if deleted.
P3 — pinned-definition rehydration skips strict binding decode (space-workflow-repository.ts:572+): getWorkflowForRun validates the pinned payload's nodes array only, never isHookBindingElement — a version pinned before the agentSlots: [] / routing-rule fixes keeps bindings the live head would decode as CORRUPT (the runtime resolver treats [] as whole-node authorization). Narrow (requires a pre-fix pinned version), but the same fail-open/fail-closed asymmetry rounds 85–87 have been closing elsewhere.
P3 — misleading export refusal for corrupt-marker + legacy combos (export-format.ts:575): the partial-coverage branch runs before the corrupt-marker branch; coverage against marker bindings always reports incomplete, so a workflow whose hook column is undecodable gets "Complete the migration first" — a migration that cannot succeed. Reorder the marker check first.
P3 — stacked stale doc comment (legacy-hook-coverage.ts:14–30): the old "distinct GATE ids" comment remains above the new per-placement one — two /** */ blocks in a row, the first contradicting the current non-deduped behavior. Delete the stale block.
P3 — accepted gap, tracked: the web __routing_unavailable__ / __legacy_hooks__ banner tests remain outstanding (acknowledged for the step-6/7 follow-up push).
Everything else from the three sub-agent sweeps (correctness, impact/compat, security, craft) verified clean at this head: no clearLegacyHooks bypass path, export refusal surfaces as a user-visible error with no partial bundle, the batch-consume sentinel cannot wedge (retry loop bounded at 3), and the m197 per-row node query is bounded by the partial-coverage candidate set. The carried P2s are quality gaps in the round-83/84 fixes, not defects in rounds 85–87 — the new code is correct as written.
Recommendation: REQUEST_CHANGES
… codex) The round-87 post-lookup rechecks in review_posted and codex_review_approved required a primary identity to EXIST — but both hooks legitimately run before any pr_ready binding stamped one, so every positive result hit current === undefined and retried forever (a found review could never pass). The recheck now retries only on a DIVERGENT identity (current !== undefined && !samePrLink): no identity means nothing to diverge from, matching the pre-recheck trust of the supplied link. Pinned by a no-stamp positive-evidence test (formal review found, refreshArtifacts empty → continue).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c5a9a666e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ent-only rechecks, RPC/test pins (round 88) P1 source-node coverage: a legacy placement that records a sourceNode now requires the covering binding's source to match — a QA→Review binding no longer migrates a Coding→Review gate (same id/method/target, different sender), which would clear the legacy definitions and leave the original handoff ungated. Refusal test pins it. P2 stamp-fail under a policy stop: a hook that stamps AND stops (pr_ready's shape) with a failed artifact write keeps the stop but converts it to override-INELIGIBLE — approving the stop previously delivered without the stamp, and the retry then read the first-writer winner's identity as a mismatch. Pinned by a pr_ready-flow test (gh seams mocked, claim throws). P2 divergent-only identity rechecks: review_posted and codex_review_approved retried forever when no pr_ready stamp existed (both hooks legitimately run before any identity is stamped); the recheck now fires only on a DIVERGENT identity. Pinned by a no-stamp positive-evidence test (via the now-exported gh test seams). P2 duplicate-delivery trade documented: the respawn owner-relaxation's known window (old-key + new-key records both delivering) is spelled out in the relaxation comment with the mitigation path. P2 test pins: the RPC-layer clearLegacyHooks strip (deleting the destructure would pass the suite while reopening the passthrough the manager reset covers) and the __firstRetryAt rollback restore (seeded + asserted, deleting the restore line now fails).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28aaf62db0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ity, post-approval hook RPCs (round 89 codex) P2 duplicate hookIds: parseHookColumn now passes the whole array to the element validator, and the binding decoder rejects rows with two individually well-formed bindings sharing one hookId — the runtime validator already rejects that shape (state is keyed (runId, hookId); duplicates share approvals/cooldowns/queued actions across routes), and such rows from earlier builds or external repairs loaded as healthy. Pinned by a duplicate-row test. P2 approval across replacement sessions: buildRetryableActionKey drops sessionId from the identity (consistent with sameRetryableActionOwner's relaxed ownership) — a respawned worker re-issuing an identical blocked action now matches the persisted __blockedActionKey, so the armed approval applies without forcing the operator to reproduce the stop and approve twice. Pinned by a replacement-session wrapper test (override + consume). P2 post-approval hook decisions: approveHook and retryHook permit operations on a DONE run when spaceTaskRepo.hasApprovedTaskForWorkflow is true — the runtime marks a run done BEFORE dispatching its post-approval worker, so the status guard alone left a live worker's displayed hook decision impossible to approve or retry. Both guards keep rejecting done runs without live approved work. Pinned by permit + still-reject tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25805bdf87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…run-start refusal (round 90 codex) P2 unsupported json type: isCustomHookElement rejects requiredData fields with type 'json' — HookDataFieldType is string/number/boolean/link only (validateCustomHooks' rule), and the prompt generator renders an unsupported type as a quoted string, handing a script expecting structured JSON a malformed value instead of the corruption marker. P2 colliding custom-hook ids: collection-level parity with validateCustomHooks after element decode — duplicate ids, a custom id shadowing a BUILT-IN hook, or a RESERVED synthetic id (a persisted __corrupt_hook_bindings__ custom hook would hijack the corruption marker's diagnostics via resolveHook's built-in-first selection). All route through the marker. Both pinned in the corrupt-shapes iteration test. P2 marker-loaded run starts: startWorkflowRun refuses to pin a marker-loaded workflow into a new run's immutable definition — repairing the head would never heal the run and every hookable action stays blocked until the task is abandoned; the start fails with the repair recipe instead (mirroring the export refusal). Local shard flake (space-worktree-manager timeout under load, passes in isolation) is the documented pre-existing one.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5414eb8d61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… exemption, meta rebinding, agents-parse guard (round 91 codex)
P2 duplicate binding ids in m197: the coverage pass rejects rows with two
individually valid bindings sharing a hookId (the repository's
collection-level decoder treats the same list as corrupt — it must defer
the drop, not count as coverage). Deferral test added.
P2 run-scoped exemption: approveHook/retryHook's done-run exemption checks
THIS run's tasks (listByWorkflowRun + status 'approved') instead of
hasApprovedTaskForWorkflow — a sibling run's post-approval task no longer
exempts a completed run's hook decisions. Permit-test updated to the
run-scoped mock.
P2 meta rebinding: scheduleQueuedRetryableActions rebinds a re-armed
record's in-memory meta.sessionId to the CURRENT owner's session — the
stale id matched no live execution, so the cancellation lookup
(getSourceNodeExecutionStatus matches agentSessionId) could never observe
the old execution being cancelled and the re-armed action kept firing for
a retired worker. The durable record's key is unchanged (the documented
duplication trade's future closure remains re-keying).
P2 agents-parse guard: m197's node-slot parsing treats a non-array
`agents` value (e.g. {}) as empty instead of letting .map() throw outside
the catch — this code runs in the migration runner's no-write precheck at
daemon startup. No-throw test added.
(The custom-hook-shadow finding was already fixed by round 90's
collection-level checks — confirmed present, replied accordingly.)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b19a140fe7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| parsed = undefined; | ||
| } | ||
| if (Array.isArray(parsed) && parsed.some((entry) => legacyHookIds([entry]).length > 0)) { |
There was a problem hiding this comment.
Fresh evidence beyond the prior [{}] fix is that a mixed array such as [validLegacyHook, {}] passes this some() check. legacyPlacements() silently skips the malformed entry, so once the valid placement has a matching v2 binding, coverage is reported complete: the runtime omits the legacy fail-closed guard and migration 197 may drop the original column even though part of it was undecodable. Require every nonempty legacy entry to be recognizable, otherwise install the corruption marker.
Useful? React with 👍 / 👎.
| // the state lookup was in flight — completing the task for the OLD | ||
| // PR while the run now points at an unmerged one. Re-resolve; a | ||
| // change refuses (the next mark_complete re-verifies the new PR). | ||
| const currentUrl = resolvePrUrl(task); |
There was a problem hiding this comment.
When a concurrent pr_ready replacement commits after this second resolvePrUrl() call but before createMarkCompleteHandler() performs its later setTaskStatus(..., 'done'), the gate still returns success and completes the task based on the old merged PR while the run now points to a potentially unmerged replacement. This re-read narrows the race but does not close it; carry an identity row/version into an atomic compare-and-complete operation, or revalidate it inside the completion transaction.
Useful? React with 👍 / 👎.
| resolveInitialPrimaryLinkUrl(runId: string): string { | ||
| // Authoritative source FIRST: the engine stamps the pr_ready-validated PR | ||
| // identity under a RESERVED hook id that no real (user-defined) hook can | ||
| // write (record_state / stateForHook target a hook's OWN id). Reading it | ||
| // outright bypasses the user-defined hook-id matching below, closing the | ||
| // colliding-hook-id / record_state PR-identity spoof for current runs. | ||
| if (!this.artifactRepo) return ''; | ||
| try { | ||
| const hookStateRepo = new WorkflowHookStateRepository(this.db); | ||
| const reserved = hookStateRepo.get(runId, PR_READY_VALIDATED_IDENTITY_HOOK_ID); | ||
| const reservedUrl = legacyPrUrl(reserved?.localState); | ||
| if (reservedUrl) return reservedUrl; | ||
| } catch (err) { | ||
| log.warn( | ||
| `resolveInitialPrimaryLinkUrl: failed to read reserved identity for run ${runId}: ${err instanceof Error ? err.message : String(err)}` | ||
| const all = this.artifactRepo.listByRun(runId); |
There was a problem hiding this comment.
When upgrading an already-approved post-approval task from the legacy engine, its authoritative PR may exist only in workflow_hook_state under __pr_ready_validated_identity__, because the old pr_ready path stamped localState.pr_url and did not require an artifact. Migration 197 deliberately defers while this work is active, but this artifact-only resolver now returns an empty URL unless the agent independently wrote a compatible artifact, so the required merge gate rejects every mark_complete attempt and strands the approved task. Retain a read-only legacy state fallback for pre-v2 runs or migrate those identity stamps to reserved artifacts.
Useful? React with 👍 / 👎.
| const hooks = raw?.hooks; | ||
| if (Array.isArray(hooks)) { |
There was a problem hiding this comment.
When an imported workflow contains a malformed legacy field such as "hooks": null, "hooks": {}, or a string, this array-only check does not reject it. Because exportedWorkflowBaseSchema does not declare hooks, Zod strips the field and validation succeeds, so the workflow is imported with no hook enforcement even though the file explicitly carried legacy hook configuration. Check field presence rather than Array.isArray() and reject every hooks key, as the comment above intends.
Useful? React with 👍 / 👎.
…n in m197 (round 92 codex) P2 interpreter: isCustomHookElement requires run.interpreter === 'bash' (the CustomHook contract and validateCustomHooks' literal) — a missing or arbitrary interpreter string previously loaded "healthy" and failed only at execution with an unsupported-interpreter error. Rejected shape added to the iteration test. P2 custom definitions in m197: legacyHookCoverage only proved the BINDING side; a legacy SCRIPT placement (instance id, no validator) matched by a structurally valid binding can still reference a custom hook the workflow's custom_hooks column does not define (or defines malformed) — dropping the legacy column would strand the workflow with a binding resolving to nothing while the executable legacy definition is destroyed. legacyPlacements now reports isValidator; m197 defers for non-built-in script placements until custom_hooks parses and defines every referenced id. Pinned by a defer-then-complete test (undefined id defers; defining the custom hook completes the drop).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a30b7e679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const customsOk = | ||
| Array.isArray(customs) && | ||
| customs.every( | ||
| (h) => | ||
| !!h && typeof h === 'object' && typeof (h as Record<string, unknown>).id === 'string' | ||
| ); |
There was a problem hiding this comment.
When a legacy script hook has a matching v2 binding but custom_hooks contains a malformed, duplicate, reserved, or built-in-shadowing definition, this check accepts it merely because each element is an object with a string id. Migration 197 then drops the executable legacy hooks column, while rowToWorkflow subsequently rejects the v2 custom-hook collection as corrupt and leaves the workflow permanently fail-closed with the original script lost. Apply the same full collection and element validation used by the repository before considering the legacy definition migrated.
Useful? React with 👍 / 👎.
| const legacyScriptIds = legacyPlacements(legacyParsed) | ||
| .filter((pl) => !pl.isValidator) | ||
| .map((pl) => pl.id) | ||
| .filter((id) => coveredIds.has(id) && !builtInIdSet.has(id)); |
There was a problem hiding this comment.
For a valid pre-v2 script hook whose instance ID happens to be pr_ready or another built-in ID, this filter excludes it from legacyScriptIds even though pl.isValidator is false. A v2 binding with that ID resolves to the built-in implementation, so coverage can pass and migration 197 can delete the legacy script while silently replacing its arbitrary business logic with unrelated built-in behavior. Legacy script placements must require an explicit custom-hook migration regardless of whether their instance ID collides with the built-in registry.
Useful? React with 👍 / 👎.
|
Closing: owning task was cancelled and nothing is driving this PR. Branch kept for reference — reopen via a fresh task if this work is resumed. |
|
Reopening: the owning task was mass-cancelled by the 2026-08-16 space-stop incident, not deliberately abandoned — the earlier closure mistook incident quiescence for dead work. Task restoration pending the update_task status capability. |
|
Closing as unworkable on current dev: 100 files and 126 commits, 1,250+ commits behind, and the hook engine landed separately in #2076. Re-slice from dev if still wanted. Branch retained. |
1 participant
Workflow hooks v2 — the two-layer model now governs MCP-action handoffs. Layer 1
Hook=requiredData+run(built-ins in@hyperneo/extensions-hooks, custom per-workflow bash scripts); Layer 2HookBindingplaces a hook on a route. Hooks own their side effects via a daemon-injectedHookContext; the return is just a flow signal (continue/stop/retry).This push finishes the cutover: the engine (4b —
executeActionresolves bindings → registry/custom-script → run → flow mapping, with engine-managed retry queueing), wires it intonode-agent-tools/task-agent-manager(4c), re-seeds the built-in workflows as bindings and makesbuildRoleSectionderive the handoff data contract generically fromrequiredData(4d), and tears out the old engine/executor/validators/connectors/types plus the legacyhookscolumn (4e). The web runtime hook-state banner is retyped to v2; the old hook editor is removed — a new editor (step 6) and engine tests + online GraphQL validation (step 7) land in a follow-up push.Also fixes blind-written GraphQL parsing in
extensions/hooksthat review surfaced:asRecord-on-array bugs that silently zeroed unresolved-thread URLs and review/comment evidence, thereview_postedviewer read from the wrong envelope level, andcodex_review_approvedis now link-driven (noGH_REPOmisresolution) and head-bound (an APPROVED review on the head SHA, or a fresh post-head-push +1 reaction), matching the codex bot in both its Bot andUser[bot]forms; GraphQL routes to the PR's host and transientghfailures retry.Green: tsc / knip / db-schema-parity / oxlint / biome clean; daemon shards pass (3 pre-existing flaky timeouts in space-worktree-manager / artifact-handler under concurrent full-suite load — all pass individually); web vitest 1562 pass.