Repository navigation
Remove Team Token PR2: Runtime actions use validated access paths - #594
josue-tech-94 wants to merge 359 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
@claude Please do one final, independent review of this entire PR at the current head. Treat this as a final merge/readiness-quality pass, not a request to re-litigate previously resolved review history. Please review the complete PR diff against its actual base and focus on:
For CodeQL specifically, please review the repository-owned analyzer changes on their technical merits. We have already spent substantial review effort proving several reported paths safe. Do not report an issue merely because stock CodeQL previously produced an alert; report it only if you can identify a concrete remaining vulnerability, false-negative risk, overly broad analyzer model, or inadequately tested security invariant. Please distinguish clearly between:
If you find a blocker, include the exact file/location, concrete failure or exploit scenario, impact, and the smallest correct fix. If you find no must-fix issue, please say so explicitly and give a final overall verdict on whether this exact PR head is safe to use as the next stacked PR base. Please review the final state as a whole rather than proposing speculative refactors or stylistic cleanup. |
|
Claude finished @josue-tech-94's task in 4m 9s —— View job Final independent review — PR #594 @
|
|
@josue-tech-94 here is an external review of PR #594. It covers exact head CodeQL: no CodeQL analysis exists for Method: 13 parallel review agents, and I verified every Blocker and High myself. Each finding is tagged with its R0 class against merge base Counts: Blocker 2 · Critical 0 · High 6 · Medium 13 · Low 17. B1, B2, H1 and H2 were already present at BlockersB1. Service images don't copy
B2. New PR2 tables have no
High
Medium
Low
Pre-existing (not counted)
CodeQL inline alerts (18)All 18 were raised on
Verified clean
Suites
Decisions for the owner
|
|
@alfredolopez80 We went through the full finding set, reproduced the actionable items, and completed the Round-5 correction pass. What we changedAll validated must-fix findings from your review are now closed:
Exact-head ordinary CI is green:
CodeQL was manually dispatched for this same exact head:
https://github.com/evenfire-ai/evenfire/actions/runs/34646387785 Claims we did not turn into fixesWe also independently rechecked the claims we classified differently. The following remain invalidated by current source/reproduction evidence rather than being silently ignored:
M6 is the same root cause as B1. L8 and the previously identified raw-hostRef / legacy-path / base access-profile observations remain pre-existing/R0 rather than PR2-introduced. These remain acknowledged but non-blocking hardening/future-boundary items:
|
|
@josue-tech-94 — consolidated multi-agent review of this PR at exact head PR #594 consolidated review — exact head
|
| Repo | evenfire-ai/evenfire |
| PR | #594 "Runtime actions use validated access paths" |
| Base ← head | feat/user-centric-access-foundation (PR1 #334, OPEN) ← feat/user-centric-runtime-access-paths |
| Author | @josue-tech-94 |
| Exact head reviewed | ad1e137ac9ca656a9616346dfe7d129c02414a5c |
| R0 merge base | 9b4755a15c92a0dc7727e72d3febbfc1cc876bb6 (current PR1 branch head) |
| Scale | 352 files, +28498 / −3447 |
| Stack position | PR2 of 4 (PR1 → dev, PR2 → PR1, PR3 → PR2, PR4 → PR3) |
| Findings | Blocker 0 · Critical 1 · High 4 · Medium 15 · Low 51 |
Method: 12 read-only agents: rpc-proxy, control-api, controllers/infra, mcp-host/WRC/external-rest-api, security, cross-service contracts, concurrency/DB/perf, CodeQL/CI, merges/docs, two test runners with mutation testing, and a prior-findings verdict pass. The concurrency/DB/perf agent was stopped before reporting (see §3). The owner's pr2-human-review-context.md was applied: R0 against 9b4755a15, §10 accepted history debt and §11 out-of-scope items are not reported as defects, and §12 is answered below. Every Critical/High finding was re-verified by the lead reviewer against the code at the exact head.
This review covers only ad1e137ac. It does not replace the internal formal acceptance, and it does not authorize merging PR1/PR2 or starting PR3.
Evidence tags: REPRODUCED = a command, test, mutant or script was run and the output is quoted. READ-ONLY = code path traced only.
Class: INTRODUCED (vs 9b4755a15) · PRE-EXISTING · ROUND5-REGRESSION (introduced by the Round-5 fixes or the PR1 sync merges).
0. Key context for reading severities
v2 cannot be enabled at this head. control-api/src/services/access/userAccessPolicy.ts:372-390 freezes reconstructionReadiness.actionContext: 'unavailable', and :222 requires 'ready'. No source path changes it, so CONTROL_API_USER_ACCESS_ACTION_CONTEXT_V2=true always throws action_context_unavailable.
Consequences:
- Every defect in a v2 runtime path is dormant at this head. It must be closed before v2 activation, not necessarily before this merge; that is the owner's call (section 6).
- Defects that break build or deploy (C1, INF-1) affect v1 traffic today.
1. Critical
C1 — rpc-proxy crashes at startup on any cluster deployment of this head
- Critical · INTRODUCED (
ef8ada2cd) · REPRODUCED + lead-verified - Location:
rpc-proxy/src/config.ts:228;deploy/overlays/minikube/configmaps/rpc-proxy-config.yaml:53;rpc-proxy/Dockerfile:20. - Defect:
mcpServerNamespace: requiredOrDevDefault('RPC_PROXY_MCP_SERVER_NAMESPACE', 'mcp-server')throws whenNODE_ENV=production, and the Dockerfile setsENV NODE_ENV=production. The variable is provisioned nowhere exceptrpc-proxy/.env.example:git grep RPC_PROXY_MCP_SERVER_NAMESPACE HEADfinds onlyconfig.tsand.env.example;- the configmap has
RPC_PROXY_HOST_NAMESPACEonly; - the variable does not exist at
9b4755a15.
- Scenario: deploying
ad1e137acwith the repo manifests makesconfigthrow at import, so rpc-proxy goes into CrashLoopBackOff and all RPC traffic stops, including v1. The run output wasconfig import failed: Missing required environment variable: RPC_PROXY_MCP_SERVER_NAMESPACE; with only that variable added, it printedconfig loaded mcpServerNamespace= mcp-server. CI did not catch this because the lanes are non-cluster. - Fix: add
RPC_PROXY_MCP_SERVER_NAMESPACE: "mcp-server"to every rpc-proxy configmap overlay. Also add a startup smoke check that loadsconfigwith the shipped configmap andNODE_ENV=production.
2. High
INF-1 — rpc-proxy image publish fails; B1 is not closed on the publish path
- High · ROUND5-REGRESSION (
0889b678b) · READ-ONLY, lead-verified - Location:
.github/workflows/build-publish.yml(rpc-proxy matrix entrypath: rpc-proxy, "Resolve build context" step);rpc-proxy/Dockerfile. - Defect: the rpc-proxy Dockerfile now copies from the repo root (
COPY packages/action-context-contracts ...,COPY rpc-proxy/package*.json ./rpc-proxy/). The matrix entry has norooted: true, so the build context stayscontext=$MATRIX_PATH(rpc-proxy/). - Scenario: the first dev publish of rpc-proxy fails with a COPY "not found" error.
assert_root_build_contextdoes not cover this entry. - Fix: add
rooted: trueto the rpc-proxy matrix entry, and extendassert_root_build_contextto fail for any Dockerfile that copiespackages/.
F1 — Rehydrated approval-suspended v2 chat task runs without live checkpoints
-
High (dormant, see §0) · INTRODUCED · READ-ONLY, lead-verified
-
Location:
mcp-host/src/core/conversation/persistence/sqliteConversationStore.ts:1014;mcp-host/src/agent/stateMachine.ts:722-726;mcp-host/src/agent/taskExecutor.ts:1138-1139, 1229-1233. -
Defect:
persistSuspendalways writessource_message: null. Cold-startbootstrap()rebuilds the task withsourceMessageundefined.taskExecutortreatssourceMessage?.authorityV2 === undefinedas legacy (if (!authorityBinding) return fallbackPort;reasoning = authorityBinding ? withRuntimeActionAuthority(...) : unguardedReasoning), and the context manager is also unguarded. -
Scenario:
- A v2 message suspends on a tool approval.
- The mcp-host pod restarts (deploy, eviction, OOM, scale-down).
- Control API revokes the path or grant.
- The approval is decided.
resumeAfterApprovalsends the tool results to the provider with no checkpoint, and every later iteration, context pass and fallback is also unchecked.
§3 invariant "every protected action revalidates live" is broken on this path.
-
Fix:
- Persist the frozen
authorityV2binding with the pending-approval row (source_messageor a dedicated column). - On rehydrate, fail closed when the task was v2 and the binding is missing.
- Add a persist → restart → resume test asserting that the checkpoint is called before the provider (liveness witness).
- Persist the frozen
N1 — 7 of the admitted delegatable operations can never be authorized; H1 is only partly closed
- High (functional; fails closed) · INTRODUCED · READ-ONLY, found independently by 2 agents, lead-verified
- Affected operations:
task.read,task.manage,model.read,model.select,session.read,session.manage(resourceruntime_session) andchat.read(resourcechat). - Location:
control-api/src/services/access/accessAuthorityStore.ts:26(the resource-type union has noruntime_session/chat);accessAuthorityStore.ts:704-836(loadDatabaseCandidateshas no branch for them);control-api/src/services/access/operationalAccessReader.ts:39-48(not inSOURCE_FAMILIES_BY_TYPE);control-api/src/services/access/liveAuthorizationResolver.ts:197, 248(not_found);control-api/src/routes/external/rpcDelegations.ts:179-183.
- Defect: issuing a delegation for these operations calls
authorizeActionV2, which always resolves tonot_found. The Round-5 H1 fix (427ace910) aligned the checkpoint destination, but the whole path still cannot be used. The fixturecontrol-api/test/fixtures/emitRuntimeSessionDelegationV2Fixture.tscallsissueUserDelegationV2directly and skips authorization, so the producer-interop tests pass on a token the real producer can never issue (§12 item 15). - Scenario: a v2 user with full access calls any
session.*,task.*ormodel.*action and gets 404 (chat.readwith ahostresource gets 400invalid_binding). - Fix: add an authority source for
runtime_sessionandchat(derived from the owning host/context grant). When doing so, bindtarget.hostRefto the session (see S1). Replace the bypassing fixture with one that goes throughauthorizeActionV2.
T-1 — Desktop "v2 cannot fall back to legacy" tests are vacuous; the Round-5 H4 proof does not hold for Desktop
- High (a claimed High closure is unproven; production code is correct) · ROUND5-REGRESSION (
e8d3e4602) · REPRODUCED independently by 2 agents; one used a cleannpm ci - Location:
rpc-proxy/src/routes/desktopProxy.test.ts:62-66(tests at :359, :478, :501, :523) vsrpc-proxy/src/config.ts:231-234. - Defect: the "valid legacy session" cookie is signed with
'test-secret-32-chars-minimum!!', but the router'sDesktopSessionServiceuses the default'dev-desktop-cookie-secret-32chars!'. The legacy path would reject the cookie anyway, so the tests cannot detect a fallback. - Evidence:
- A witness with the test cookie got
401 {"error":"Invalid desktop session"} proxied 0; a router-signed cookie got200 proxied 1. - Mutants H4a (HTTP declared-v2 → legacy), H4b (WS unverifiable → legacy) and H4c (WS denial → legacy) each SURVIVED the full suite (575/575).
- With the secret env corrected, 5 tests fail as they should.
- The HTTP stale/unavailable/mismatched tests (:377, :394) remain unproven.
- A witness with the test cookie got
- Fix:
- Mint the cookie with the router's secret (set
RPC_PROXY_DESKTOP_COOKIE_SECRETbefore import). - Add a positive control: the same cookie is proxied on a non-v2 request.
- Assert the v2 status and body, not only "proxy not called".
- Mint the cookie with the router's secret (set
3. Medium
| ID | Class · Evidence | Location | Defect → scenario | Fix |
|---|---|---|---|---|
| T-B1 | INTRODUCED · REPRODUCED (mutant M11c survived) | mcp-host/src/runtime/actionAuthority.ts:124-127 |
The continueWithToolResults checkpoint is untested; resumeAfterApproval sets skipContextManager=true. Removing it means a provider call after revocation with the suite green. |
Adapter test allowed→denied, plus a taskExecutor resume test with a witness. |
| T-2 | ROUND5-REGRESSION · REPRODUCED (H3g, RP-3 survived 575/575) | rpc-proxy/src/routes/desktopProxy.ts:265-266, 315-316; sandboxUi.ts:845-846 |
No test proves the active-view lease closes on disconnect. H3 is PARTIAL. Dropping the registrations leaks lease timers that keep checkpointing closed views. | Emit close/finish/error per surface and assert lease.close exactly once, with a started-lease witness. |
| CQ-F1 | INTRODUCED · READ-ONLY | .github/codeql/queries/EvenfireFileAccessToHttp.ql:63-72 |
writesBaseUrlThroughRuntimeAuth casts a module-scoped let read to SourceNode, so it is always empty. The UnsafeProperty/UnsafeAlias fixtures use concatenation, which hasFixedReadinessDestination rejects, so they are vacuous. runtimeAuth.baseUrl = request.body.url plus a template-literal fetch would be suppressed. Production code is safe today. Re-opens M8. |
Model the write via DataFlow::PropWrite and add a template-literal unsafe fixture that must keep a finding. |
| CQ-F3 | INTRODUCED · READ-ONLY | EvenfireMissingRateLimiting.ql:419-427, 462-480 |
The OAuth suppression accepts by callee name plus line order; an ignored result or a local stub returning true suppresses, and the fixture's "safe" case is such a stub. The WFC branch accepts any receiver .use without dominance or await. Re-opens M9. |
Require the guard result to dominate the sink; replace the stub fixture with an unsafe variant that must alert. |
| CQ-F5 | INTRODUCED · READ-ONLY | EvenfireMissingRateLimiting.ql:298-326 |
The v2ViewAuthority branch trusts the middleware by name, but that middleware passes non-v2 requests through (desktopProxy.ts:44-50, sandboxUi.ts:106-112), so the suppression rationale does not hold for the legacy path. There is no fixture. |
Limit the branch to handlers that fail when authorizedActionV2 is absent; add a fixture. |
| CQ-F6 | PRE-EXISTING config, process · READ-ONLY | .github/workflows/codeql.yml |
CodeQL triggers only on [main, dev], so no analysis exists for ad1e137ac. The last dispatch was run 34646387785 on 72baaf513 (10/10 QL tests). The merged PR1 code at this head is unscanned. |
Dispatch CodeQL on ad1e137ac before acceptance; consider adding feat/**. |
| TEST-1 | PRE-EXISTING harness, relied on by Round-5 · REPRODUCED | scripts/tests/test-control-api-runtime-access-contract.sh:12 |
It asserts 91 but the head has 105 (EXIT=1, no message); it also failed at base (88 vs 102). It is not wired into CI or the Makefile. Round-5 cited it as B2 proof, but it cannot go red where it matters. |
Fix the count (or derive it) and wire it into CI. |
| CA-F1 | INTRODUCED · READ-ONLY | control-api/src/db.ts:3137-3153 (migration 010f) |
A non-CONCURRENTLY CREATE INDEX on workflow_runs plus 4 ADD COLUMN … REFERENCES inside the bounded migration transaction (lock 10 s / statement 15 s). This contradicts D34's PR1_ONLINE_INDEX_PLAN. On a large table the migration aborts or blocks writes. |
Move the index to the online-index plan (CONCURRENTLY, outside the transaction) and extend the policy test to PR2 migrations. |
| CA-F2 | ROUND5 partial (M4 residual) · READ-ONLY | control-api/src/services/workflowRunService.ts:204-211; workflowTriggerApprovalService.ts:76-80 |
reauthorize() inside withTransaction takes a second pool connection. With CORE_POOL_MAX (10) concurrent triggers the pool is starved and requests deadlock until timeout. R5-M2 fixed only recordDecision. |
Reauthorize before opening the transaction, or pass the transaction client through. |
| INF-2 | INTRODUCED · READ-ONLY | .github/workflows/build-publish.yml paths-filters |
packages/action-context-contracts/** is missing from the filters of the 4 consumer images, so a contract-only change publishes no image. |
Add the path to each consumer filter. |
| INF-3 | INTRODUCED · REPRODUCED | scripts/tests/test-dockerfile-local-packages.sh (~197-209) |
The guard skips workflow-recipes. Process substitution loses the exit status, so 0 packages or a broken manifest passes (harness cases 2 and 4). | Fail on an empty package set, propagate the exit status, and include workflow-recipes. |
| M3 | INTRODUCED · REPRODUCED (author: invalidated) | control-api/src/services/access/pr2ReadinessEvidence.ts:275-294, 532; control-api/src/main.ts:69-70 |
ON CONFLICT DO UPDATE … updated_at = NOW() runs even when nothing changed. Against real PG15 a restart dropped 17/17 → 0/17 ready. Once v2 is on, main.ts resolves policy before its own reporter runs, so every restart throws, leading to a crash loop. |
Add WHERE (…) IS DISTINCT FROM (EXCLUDED.…); add an identical re-import test. |
| M5 | INTRODUCED · READ-ONLY, dormant (author: invalidated) | control-api/src/middleware/pr2ReadinessWriterAuth.ts:63-70 |
Any valid mcp-host access JWT, including a WRC recipe-pod token, is accepted as the fleet-wide mcp-host readiness writer. It can post failed to hold hops down or passed with no real fleet. The test at middleware.pr2ReadinessWriterAuth.test.ts:62 asserts the permissive behaviour. §12 item 12 is YES (dormant). |
Dedicated writer token or readiness-only scope for first-party Host CRDs; flip the test. |
| T-7 / C3 | INTRODUCED · READ-ONLY | rpc-proxy/src/actionAuthorityV2.test.ts:114,180,224; session-search-v2.test.ts:87; mcpProxyService.v2.test.ts; mcp-host/src/runtime/actionAuthorityCheckpointClient.test.ts:77; WRC workflowActionCheckpointClient.test.ts:65 |
Every checkpoint-response fixture, including destination, is written by hand. Producer drift of the H1 kind stays green; C2 below is exactly such a drift. §12 item 15 is NO for checkpoint responses. |
Add a control-api checkpoint-response emitter (real resolveActionDestination) and consume it in the rpc-proxy, mcp-host and WRC tests. |
| RP-5 | PRE-EXISTING (5d663a496) · REPRODUCED |
rpc-proxy/src/routes/sandboxUi.ts:819-820 |
Shared proxy .once('proxyReq'/'proxyRes') listeners race, so concurrent responses can lose CSP/XFO and the off-origin redirect filter. Not a PR2 blocker under R0. |
Per-request listeners (selfHandleResponse or request-scoped handler). |
Coverage note: the dedicated concurrency/DB/performance agent was stopped before its report, by owner decision, because it could not change the merge recommendation. Its scope is covered by other agents: CA-F1 (migration locking), CA-F2 (pool starvation), §8.3 (WRC checkpoint inside the lock), RP-2/T-2 (lease leaks) and RP-5 (proxy listener race). M2 (performance follow-up) and the decide→consume race analysis were not independently re-verified in this review.
4. Low
| ID | Class · Evidence | Location | Summary |
|---|---|---|---|
| S1 | INTRODUCED · REPRODUCED (contract), latent | packages/action-context-contracts/index.cjs:625-643; rpc-proxy/src/routeActionBindingV2.ts:270-300; control-api/src/services/access/actionDestination.ts:36-48 |
For runtime_session/chat.read, target.hostRef is not bound to resource.logicalId (task.manage session-a + mcp-host/other is accepted; chat.message.invoke rejects). It becomes cross-host authority once N1 adds a grant source. |
| S2 | INTRODUCED · READ-ONLY | gfs-controller/src/api/serve.ts:335-336 |
requireExactGfsAuthority compares binding.target with itself, so the check is vacuous; it would stay green if the route-level checks were removed. Build the expected target from the request. |
| S3 | INTRODUCED · READ-ONLY, fails closed | index.cjs:716-722; serve.ts:916-920 |
A v2 GFS copy checkpoints gfs.write on the source, not the destination parent, so legitimate copies are denied. |
| C2 | INTRODUCED · REPRODUCED, dormant | rpc-proxy/src/actionAuthorityV2.ts:135-157 vs control-api/src/services/access/actionDestination.ts:50-51 |
chat.read: the producer returns destination null, rpc-proxy expects host (44 rows compared, 2 mismatches; mcp.catalog.read is the reverse, informational). Move the destination rule into the shared package. |
| C4 | INTRODUCED · REPRODUCED, latent | control-api/src/services/access/actionAuthorityCheckpoint.ts:99; workflow-recipes/src/reconciler/dbRunProcessor.ts:122-130 |
The checkpoint parser requires localeCompare key order, while WRC sends a JSONB read-back. It works only for workflow.trigger; workflow.approval.consume would get 400. |
| C5 | INTRODUCED · READ-ONLY | gfs-controller/src/auth/actionAuthority.ts:242-316; workspace-files-controller/src/auth/actionAuthority.ts:145-235 |
GFSC/WFC re-implement the checkpoint client with no shared validator, and every non-5xx collapses to 403. GFSC clears its abort timer (:285) before response.json() (:289), so a stalled body has no timeout (INF-4). |
| C6 | INTRODUCED · READ-ONLY | pr2ReadinessEvidence.ts:511-532 |
If WFC/GFSC is rolled back to a pre-PR2 image, runtime evidence stays ready for up to maxRuntimeEvidenceAgeSeconds (≤86400), and the old verifiers accept v2 tokens without a checkpoint. Use a claim that old verifiers reject (aud/crit). |
| F2 | INTRODUCED · READ-ONLY | mcp-host/src/server/routes.ts:304, 354-366 |
Caller body.authorityV2 is not deleted on the legacy paths (current callers allowlist their bodies). |
| F3 | INTRODUCED · READ-ONLY | mcp-host/src/activityHub.ts:81 |
Activity snapshot/SSE expose the stored binding (sid, delegationJti, accessPathId) to legacy and read_all readers. |
| F4 | INTRODUCED · READ-ONLY | mcp-host/src/core/reasoning/port.ts:76-89 |
A fallback-provider RuntimeActionAuthorityError becomes a non-retryable LlmError(ApiCallFailed), losing authority_unavailable. |
| F5 | INTRODUCED · READ-ONLY | workflow-recipes/src/reconciler/workflowActionCheckpointClient.ts:79-81, 117-130; dbRunProcessor.ts:157-164, 499-507 |
Clock skew on validUntil fails a run permanently; a non-trigger binding is retried forever; no failure reason is persisted. |
| F6 | INTRODUCED · READ-ONLY | external-rest-api/src/routes/gfs.ts:305-315; contextSharedFilesystems.ts:99 |
WorkflowActionDelegationTransportError returns 500 instead of 400 on the GFS token and shared-filesystem routes. |
| CA-F3..F8 | INTRODUCED | control-api | F3 orphan approval_consume binding (userApprovalRequestService.ts:1043-1064). F4 WorkflowApprovalAuthorityRequiredError unmapped → 500 (decisions.routes.ts:187-210). F5 idempotency hash includes delegationJti → retry 409. F6 REPRODUCED: internal/oauth.ts:70 accepts a non-date expiresAt as unexpired. F7 actionAuthorizer.ts:206-207 swallows without a log. F8 derived sourceExpiresAt ≤ sourceIssuedAt → CHECK 23514 → 500. |
| RP-2 | INTRODUCED · REPRODUCED | sandboxUi.ts:842-846; desktopProxy.ts:262-266, 313-316 |
Lease close listeners are registered after await; a client abort during the checkpoint leaves the lease polling every 10 s until delegation expiry (~29 calls). |
| RP-4 | INTRODUCED · REPRODUCED | desktopProxy.ts:313 |
Lease denial destroys the client socket but not the upstream proxy socket to the desktop pod. |
| RP-6/7/8 | INTRODUCED · READ-ONLY | routeActionBindingV2.ts:377-385; wakeAndHold.ts:165 |
RP-6: runtimeHostEdgeContext returns a legacy context when the destination is missing (unreachable). RP-7: no producer→authorizeActionV2 test. RP-8: endsWith hostRef match; Desktop strips only x-clerum-edge-*; 403 upgrade text says "Unauthorized". |
| T-3 | INTRODUCED · REPRODUCED (L5c survived) | desktopProxy.ts:366 |
Legacy Desktop WS header strip is untested. |
| T-4 | INTRODUCED · REPRODUCED (X-H2 survived) | control-api/src/routes/external/workflows/runs.routes.ts:99 |
The entityType literal is not tied to the 0113 CHECK list. |
| T-5 | INTRODUCED · REPRODUCED (M12c survived both lanes) | pr2ReadinessEvidence.ts:530 |
The future-dated evidence clause (runtimeAge >= 0) is untested. |
| T-6 | INTRODUCED · READ-ONLY | .github/workflows/ci-public.yml |
db.realPostgresMigration.integration (the only B2 runtime-role proof) and gfsUploadSession.realPostgres are not in the pinned critical-suite list. |
| T-8 | INTRODUCED · READ-ONLY | external-rest-api/src/pr2ReadinessReporter.test.ts:40 |
The reporter body is never parsed by control-api's real parser. |
| T-B2..B4 | INTRODUCED; B3 REPRODUCED | WRC dbRunProcessor.ts:116-164; GFSC actionAuthority.test.ts:12-64; WFC :30-66 |
T-B2: WRC FOR UPDATE/FAIL_PENDING_AUTHORITY only mocked. T-B3: GFSC/WFC fixtures use ath1_/abh1_ hashes that the shared validator rejects, and HASH_RE is looser than the contract. T-B4: weak liveness witnesses in activityRoutes.v2.test.ts. |
| L1 | INTRODUCED · REPRODUCED (author: invalidated) | packages/action-context-contracts/index.cjs:182 |
localeCompare makes the hash locale-dependent (da_DK differs; cs/sk/haw reorder real key sets). Fails closed. Use a code-point comparator. |
| L3 | INTRODUCED · REPRODUCED (author: invalidated) | workspace-files-controller/src/auth/actionAuthority.ts:229-231 |
The abort DOMException (code=20) matches 'code' in error, so a checkpoint timeout returns 500 instead of 503. |
| L12 | INTRODUCED · READ-ONLY (author: invalidated) | workflow-recipes/src/reconciler/dbRunProcessor.ts:492-495 |
No test covers a bound run without a checkpointer. |
| CQ-F2/F4/F7/F8 | mixed | .github/codeql/**, scripts/tests/test-dockerfile-local-packages.sh |
F2: header/URL flow uses local SourceNode. F4: readiness-writer branch has no fixture (§8.5); the nullary issuer predicate is global; UserControlledBypass.ql uses line order rather than dominance. F7: the test step does not assert count ≥10. F8 REPRODUCED: vacuous pass on 0 packages. |
| REL-1 | ROUND5-REGRESSION · REPRODUCED | host-connectivity-controller version |
The PR2 HCC bump was lost in the sync merges: head 0.1.322 equals PR1 head 0.1.322, although HCC source differs. Bump to 0.1.323. |
| LOG-1 / L8 | INTRODUCED (1 line, commit 41db179dc) |
mcp-host/src/agent/stateMachine.ts:427 |
Added direct console.log. mcp-host's console is overridden by the structured wrapper, so this is a repo-rule nit, not a leak. |
| MAINT-1..3 | INTRODUCED | reporters, actionContextV2.ts:19-20, operationTarget.ts:175, gfs-controller |
MAINT-1: 6 near-copy readiness reporters hardcode 'pr2-readiness-v1'. MAINT-2: unused exports. MAINT-3: 6 gfs-controller files reformatted to no-semicolon. |
| DOC-1..3, HYG-1..3 | process | PR body, docs | DOC-1: the PR body is stale (names head 6a14a73a4, says "draft" although isDraft=false, omits the ad1e137ac merge). DOC-2: no R5-id ↔ review-id mapping published. DOC-3: zero docs changed (WFC/GFSC READMEs stale; readiness operator env vars undocumented; specs 24/25/42/43, tracker and ledger absent from repo history). HYG-1..3: commit-grouping notes. |
| T-9, T-10 | PRE-EXISTING / flaky | desktopProxy.test.ts:301; external-rest-api/test/routes.gfs.test.ts |
T-9: the cookie-hostRef test fails on the signature, not the hostRef. T-10: socket hang up under full-suite load (31/31 in isolation ×3). |
5. Verdicts on findings already raised in this PR
The mapping of Round-5 ids to review ids was recovered from the commits: R5-B1→B1 0889b678b, R5-B2→B2 edec1e7bc, R5-H1→H1 427ace910, R5-H2→H2 4d2c128ab, R5-H3→H3 a32b58986, R5-H4→H4 e8d3e4602, R5-H5→H5 838ba8c5f, R5-H6→H6 135d6a803, R5-H7→L5 1c561e909, R5-M1→M1 43f4e91d1, R5-M2→M4 72baaf513, R5-M3→M7 d7dc246e6, R5-M4→M10 c497d20ce, R5-M5→M11 f41817f64 (+ fixup 4235d1ab1), R5-M6→M12 479b5ae2b.
5.1 Items the author marked CLOSED
| ID | Verdict at ad1e137ac |
Evidence |
|---|---|---|
| B1 | NOT CLOSED (publish path) | INF-1; M6 PARTIAL |
| B2 | CLOSED | Real-PG grant mutants B2a–d KILLED. The shell contract test it cited is broken and not in CI (TEST-1). |
| H1 | PARTIAL | Destination aligned (44-row matrix); authorization is unreachable (N1). |
| H2 | CLOSED | Migration 0113 covers every entity_type producer; real-PG test kills the widened CHECK. |
| H3 | PARTIAL | Lease mount/deny mutants H3a–f KILLED; close-on-disconnect H3g/RP-3 SURVIVED (T-2). |
| H4 | Production code CLOSED, proof NOT valid for Desktop | T-1: H4a/b/c SURVIVED; Sandbox H4d KILLED. |
| H5 | CLOSED | Real client with 503 → no child, ROLLBACK witness; mutant KILLED. |
| H6 | CLOSED | H6a/b KILLED. |
| L5 | CLOSED (v2 paths) | L5a/b/d–g KILLED; legacy WS strip untested (T-3). |
| M1 | CLOSED | Permanent denial terminalizes and is not re-checkpointed; mutant KILLED. Edges in F5. |
| M4 | PARTIAL | recordDecision fixed; trigger reauth still uses a 2nd pool connection (CA-F2). |
| M7 | CLOSED | Wiring survives both sync merges. |
| M10 | CLOSED | GFSC ×9 and WFC ×9 mutants KILLED. |
| M11 | CLOSED for fallback/retry (M11a/b KILLED) | The continueWithToolResults checkpoint is untested (T-B1); rehydrate bypass (F1). |
| M12 | CLOSED with gaps | M12a/b KILLED in real-PG only; runtimeAge >= 0 untested (T-5); inclusive boundary untested. |
5.2 Items the author marked INVALIDATED
| ID | Verdict | Note |
|---|---|---|
| M3 | STILL VALID (REPRODUCED) | Section 3 |
| M5 | STILL VALID (dormant) | Section 3 |
| M8 | RE-OPENED with new evidence | CQ-F1 |
| M9 | RE-OPENED with new evidence | CQ-F3 |
| L1 | VALID (latent, fails closed; REPRODUCED) | Section 4 |
| L2 | Invalid | Names are NFC-enforced at creation; GFS binds a UUID. |
| L3 | STILL VALID (REPRODUCED) | Section 4 |
| L4 | Valid observation, not a defect | Renew is bounded by the 8 s upstream timeout; residual only if it is set above 30 s. |
| L6 | Valid observation, not a defect | Only the confined vs unconfined pairs need the distinctness check. |
| L10 | Low hardening | Callers include per-host mcp-host JWT holders; work is bounded by AccessExecutionBudget. |
| L12 | STILL VALID (test gap) | Section 4 |
| L13 | Invalid at head | 4d2c128ab kills the mutant. |
| L16 | Partly re-opened | CQ-F4 (no readiness-writer fixture). |
| L17 | Invalid (REPRODUCED) | Node decodes header values as latin1, so .length equals bytes; Buffer.byteLength(…,'utf8') would be wrong. |
5.3 Items the author accepted as non-blocking follow-ups
Settled; recorded here with their state at head.
| ID | State |
|---|---|
| M2 | Not re-verified (see the coverage note in §3). |
| M13 / claude §8.2 | The claim is incorrect: 5- and 6-key write bindings pass the WFC target check (REPRODUCED). Nothing to fix. |
| L7 | Present: 5 of 6 reporters swallow errors silently (only WRC logs). |
| L9 | Present and wider than reported: GFSC and WFC also hardcode the checkpoint path. |
| L11 | Present, mitigated: jti reuse still raises an unmapped 23505. |
| L14 | Present. |
| L15 / claude §8.1 | Unresolved: mcp-host does not re-compare attribution; GFSC/WFC do. |
5.4 claude[bot] observations (§8 of the owner context)
| Item | State |
|---|---|
| §8.1 mcp-host attribution | Unresolved (L15). |
| §8.2 WFC write target | Claim incorrect. |
§8.3 WRC checkpoint inside lock + FOR UPDATE |
Unresolved (dbRunProcessor.ts:475, 497). |
| §8.4 UTF-16 header length | Claim incorrect (L17). |
| §8.5 readiness-writer CodeQL fixture | Unresolved (CQ-F4). |
5.5 Pre-existing / R0 items
| Item | Verdict |
|---|---|
| Legacy raw-hostRef SSRF sink | PRE-EXISTING, not worse (desktopProxy.ts:259, 364-367 equal base :200/:249). |
Legacy branch of v2OrLegacyHostAllowed |
PRE-EXISTING, not worse. |
authorization_catalog_environment TSV |
PRE-EXISTING, 0 changes. |
| L8 console.log | INTRODUCED (1 line), not pre-existing (LOG-1). |
5.6 The 18 CodeQL inline alerts (all currently dismissed)
Lines were resolved through the code-scanning API; the inline line numbers are stale.
| Alert | Head location | Verdict |
|---|---|---|
| 1340 / 1341 / 1342 | rpc-proxy/src/routes/rpc.ts:784-788 GET /rpc/hosts/:hostRef/sessions/search |
True positive, new route (absent at base; the author labelled it pre-existing). Authenticated + requireScope + v2 checkpoint; acceptable by design. Note that rpc-proxy has no limiter (pre-existing), and PR2 adds this route and /desktop/:hostRef/reconnect. |
| 1312 / 1313 | desktopProxy.ts:179-187 POST /session |
PRE-EXISTING route. |
| 1314 / 1315 | desktopProxy.ts:188-196 POST /reconnect |
New, v2 only, covered by the throttling decision. |
| 1316 | desktopProxy.ts:239 view ALL |
PRE-EXISTING route, v2 guard added. |
| 1292 (SSRF) | desktopProxy.ts:317-320 v2 WS upgrade |
New v2 sink (not pre-existing); guard holds: canonicalHostRef rejects / and \, NAMESPACED_REF_PATTERN, signed target hash. |
| 1302 / 1303 | control-api/src/routes/internal/actionAuthorityCheckpoint.ts:24-25 |
By design (L10). |
| 1304 | actionAuthorityHostWake.ts:32 |
False positive (rateLimitMiddleware at :33-40). |
| 1305 | pr2ReadinessEvidence.ts:14 |
By design, but the guard is weak (M5). |
| 1307 / 1308 / 1309 | control-api/test/* |
Test-only. |
| 1296 | mcp-host/src/runtime/pr2ReadinessReporter.ts:29-36 |
By design (credential to the configured Control API base + fixed path). |
| 1362 | rpc-proxy/src/routes/sandboxUi.ts:686 |
False positive (isV2ViewRequest needs middleware-set state; the legacy cookie is verified as at base). |
6. Decisions for the PR owner
- Dormant v2 defects vs merge. F1 (High), N1 (High), M3, M5 and CA-F2 are unreachable while
actionContextis frozen'unavailable'. Decide whether PR2 must close them, or whether they become tracked preconditions for v2 activation. Recommendation: fix F1 and N1 in PR2. They are PR2's own invariants (§3 live revalidation; H1 closure) and PR3 builds on them. - INF-1 severity. It breaks the first dev publish of rpc-proxy, not a live service. Recommendation: fix together with C1 (two small config edits).
- CodeQL on the exact head. Decide whether acceptance requires a CodeQL dispatch on
ad1e137ac(CQ-F6) and whether the re-opened M8/M9 query weaknesses must be fixed in PR2. - Re-opened author invalidations. M3, M5, L1, L3 and L12 have new reproductions or exact code paths. They need an owner verdict.
- Rollback posture (C6). Decide whether "old verifier accepts v2 tokens" during the evidence-age window is acceptable for this rollout.
7. §12 checklist (consolidated)
| # | Question | Answer | Where |
|---|---|---|---|
| 1 | Untrusted input creates authority | NO | rpcDelegations.ts issues only after live authorizeActionV2; strict edge envelope (edgeRuntimeAuth.ts:74-203). Defense-in-depth gap F2. |
| 2 | Session/team/path claim bypasses live authorization | NO for request paths | loadPrincipalAuthoritySnapshot; checkpoint re-evaluates. Exception: F1 rehydrate. |
| 3 | Delegated operation/target widens downstream | NO today | routeActionBindingV2.ts:274-299, edgeRuntimeAuth.ts:268-283, GFSC/WFC exact compare. Latent: S1. |
| 4 | Failed v2 reaches legacy behavior | NO in production code | middleware/auth.ts:34-57, mcpProxyService.ts:100-111, desktopProxy.ts:289-326. The Desktop tests do not prove it (T-1). |
| 5 | Protected effect missing live checkpoint | YES (dormant) | F1. All other effects are covered. |
| 6 | Service credentials substitute user authority | NO | actionCheckpointCaller.ts:60-79; distinct tokens enforced by apply-inter-service-tokens.sh. |
| 7 | Stored provenance becomes bearer authority | NO | uploadSession.ts:322-331, uploadFinalizer.ts:129-134. Provenance exposure F3. |
| 8 | Workflow child transitions broader than the single edge | NO | WRC accepts only workflow.trigger (workflowActionCheckpointClient.ts:79-81); decide→consume is the only in-process edge. |
| 9 | Sandbox/RDP outlives revocation bound | NO at default config | activeViewLease.ts:3-4, 48-58 (10 s / 30 s); residual only if upstream timeout >30 s. Lease leaks RP-2/T-2 do not extend authority. |
| 10 | OAuth stale/duplicate/wrong recipe-client | NO for v2 | oauth.ts:47-87, recipeOAuthClient.ts:10-16. Header-absent path equals legacy base. CA-F6 date parsing is Low. |
| 11 | Filesystem crosses user/path/target | NO | WFC exact target (actionAuthority.ts:152-161); GFSC route checks (serve.ts:290-331, 547-550, 916-920). S2 vacuous duplicate check; S3 copy fails closed. |
| 12 | Readiness true with incomplete/stale evidence | YES (dormant) | M5 writer spoofing. Also: heartbeat outcome hardcoded 'passed', C6 rollback window, M3 false negative. |
| 13 | Custom CodeQL models hide unsafe variants | YES | CQ-F1, CQ-F3, CQ-F4, CQ-F5. |
| 14 | Tests fail when protection removed | PARTIAL | Killed: H3a–f, H4d, H5, H6, L5, M1, M10, M11a/b, M12a/b (real-PG), B2. Survived: H4a/b/c (T-1), H3g/RP-3 (T-2), M11c (T-B1), L5c, M12c, X-H2; untested: M3, M5, L12. |
| 15 | Cross-service fixtures emitted by real producers | PARTIAL | Delegation JWTs: yes. Checkpoint responses (T-7), readiness bodies (T-8), GFSC/WFC fixtures (T-B3): no. The runtime_session fixture bypasses authorization (N1). |
8. Verified clean
- Sync merges:
6a14a73a4andad1e137acwere re-created and produce identical trees, so there is no evil merge. The only conflicts wereexternal-rest-api/src/releaseManifest.tsand thecontrol-apipackage/lock.validate-release-version-bumpsEXIT=0. - Delegation JWT:
- RS256 only, with issuer and audience; exact claim set; TTL ≤300 s; iat skew ≤30 s.
- Canonical target with matching hash.
- Separate audiences for GFS, WFC and legacy RPC tokens.
- Canonicalization (REPRODUCED, 15/15): key order does not change the hash;
__proto__, arrays, non-strings, nested objects and control characters are rejected; NFC and NFD forms hash differently, so there is no collision; trimming is applied consistently before both check and effect. - Headers:
x-clerum-edge-*is stripped on all HTTP routes and beforeproxy.wson both WS branches. The Sandbox proxy strips cookie, authorization andx-clerum-*. The delegation header is in both redaction lists. - No v2 → legacy fallback in production code in rpc-proxy, mcp-host, external-rest-api, GFSC or WFC.
- Service tokens: callers are confined by resource type and operation. GFSC/WFC/RPC tokens must be distinct. Comparisons are timing-safe. Every internal route after the gate also requires a named service.
- OAuth resolves exactly one recipe client before any secret use; no
oauthClients.findremains. - Operation registry: 43 ids equal 43 wire definitions. Nothing routed by rpc-proxy is outside the registry. Request shape (11 keys) and header names are consistent across 5 consumers.
- Mixed-version rollout: new consumers with an old control-api fail closed everywhere.
- v2 enablement is gated on all 17 hops, and runtime evidence requires
image_revision == activated source. - Hygiene: no placeholders; no masking
|| true/continue-on-errorin gates; CodeQL suite exclusions paired with same-@idreplacements.
9. Suites and mutation runs
All runs used Node 24.18.0 with npm ci (scripts enabled). Counts are what executed; lint was not run.
| Service | Result |
|---|---|
packages/action-context-contracts |
9 / 9 pass |
| rpc-proxy | 144 files · 575 / 575 pass · tsc EXIT=0 |
| control-api unit | 527 files · 5547 pass · 0 fail · 293 skipped (real-PG-gated + 3 network) · tsc EXIT=0 |
| control-api real-PG (PG16) | 55 files · 292 / 292 pass · all 30 CI critical suites present |
| external-rest-api | 45 files · 308 / 309 (1 flaky routes.gfs.test.ts socket hang up; 31/31 in 3 isolated reruns) |
| workflow-recipes | 2819 pass · 29 skipped |
| gfs-controller | 631 pass · 9 skipped · real-PG lane 9 / 9 |
| workspace-files-controller | 121 pass |
| host-connectivity-controller | 2376 pass |
| mcp-host | 3795 pass · 1 todo (pre-existing) |
| Mutation runs | tests-b: 26 / 27 killed (M11c survived). tests-a: see §7 row 14. Tree restored and verified clean after each mutant. |
| CI | Run 34863665613 on ad1e137ac: 37 checks pass. No CodeQL analysis exists for this head (CQ-F6). |
10. Recommendation
Do not merge at ad1e137ac yet.
Fix before merge:
- C1: rpc-proxy would not start on any cluster deployment.
- INF-1: rpc-proxy image publish is broken, so B1 is not actually closed.
Both are small configuration edits.
Strongly recommended in PR2, because they are PR2's own invariants and Round-5 closure claims:
- F1: checkpoint loss after rehydrate.
- N1: runtime_session/chat authorization is unreachable.
- T-1: the Desktop H4 proof is vacuous.
Can be follow-ups tracked before v2 activation: the Medium items, especially CA-F1/CA-F2, M3, M5, the CodeQL query weaknesses, and a CodeQL dispatch on the final head.
Low items are optional hardening.
Process: PR1 #334 is still OPEN on GitHub (not merged to dev); its branch has only been merged into PR2, twice. The PR body should be refreshed to name ad1e137ac (DOC-1).
|
@claude Please do a final independent review of this entire PR at the current head and determine whether PR2 is safe and complete as the base for PR3. Context:
The latest acceptance coverage also specifically proves the original team-derived Host problem:
Please review the complete effective PR2 diff against its current PR1 base, including:
PR3 handoff contextIf PR2 is sound, PR3 will be stacked on this PR and will perform the Desktop cutover. PR3 is expected to:
Please explicitly conclude one of these:
Do not review or design PR3 implementation itself beyond identifying a concrete PR2 contract or behavior that PR3 would be unable to rely on. Also do not treat PR1 as merged into |
|
Claude finished @josue-tech-94's task in 6m 7s —— View job Final independent review — PR #594 @
|
| Prior finding | Status @ 4b8147a8 |
Evidence |
|---|---|---|
C1 rpc-proxy crashes on cluster start (RPC_PROXY_MCP_SERVER_NAMESPACE unprovisioned under NODE_ENV=production) |
Closed | Now set in deploy/base/rpc-proxy/rpc-proxy.yaml:35; guarded by rpc-proxy/src/config.hccBaseUrl.test.ts:71-73 asserting both the config default and the deployment env. |
| INF-1 rpc-proxy image publish COPY-not-found (build context not root) | Closed | build-publish.yml rpc-proxy matrix entry now has rooted: true (:284-287); the "Resolve build context" step sets context=. for rooted images. |
| T-1 Desktop "no v2→legacy fallback" tests vacuous (cookie signed with wrong secret) | Closed | Tests now sign with config.desktopCookieSecret (desktopProxy.test.ts:64) and add a positive control ("proxies a valid legacy-only desktop session", :286), so the "does not downgrade" cases can actually fail. |
N1 runtime_session/chat operations unauthorizable (not_found) |
Closed | authorizationResourceForAction (actionAuthorizer.ts:65-77) rewrites runtime_session/chat to the owning host via target.hostRef before authorization; loadSimpleOperationalGrantCandidates (accessAuthorityStore.ts:412-426) grants the host chat.read, task.read/manage, model.read/select, session.read/manage. Original resource identity is retained in the issued token (:196-201). |
| F1 rehydrated approval-suspended v2 task runs without live checkpoints | Closed | Binding is persisted with the pending-approval row (sqliteConversationStore.ts:1041), restored on bootstrap (stateMachine.ts:723-727), and the rehydrate path fails closed on a missing/corrupt binding (sqliteColdStartLoader.ts:164 + strict validRpcApprovalSource). Proven by crossPodRestart.test.ts (round-trip + corrupt-provenance drop) and taskExecutor.test.ts:321-373 (checkpoint runs before the provider on tool-result continuation; denial → access_path_stale). This also closes the related T-B1/M11c (continueWithToolResults) coverage gap. |
2. Team-derived Host contract (the core PR3 dependency) — sound
The specific "team-only Host access" invariants PR3 relies on are implemented correctly and, for the security decision, genuinely proven:
- User stays the authenticated principal; team is only provenance. Access paths are signed with
principalUserId, never the team (accessPath.ts:60,71-99);teamIdis a carried provenance field only. The runtime edge setstrustedEdgeContext.userId = claims.subwithpathKind/effectiveTeamIdas separate provenance, and the interop test explicitly assertstrustedEdgeContext.userId === userIdand!== teamId(userDelegationV2.producerInterop.test.ts:205-206). - Team-derived Host authorization is resolved live.
accessCatalogFamilies.realPostgres.integration.test.ts:590-670drives the realbuildAccessCatalog+resolveLiveAuthorizationagainst real Postgres; the team candidate comes from an actualteam_agents JOIN team_members … status='active'query (accessAuthorityStore.ts:368-373). - Live revocation denies. Same real-PG test sets
team_members.status='deleted', then asserts the catalog returns[]and the resolver returnsnot_found(:649-669). The commit chain (d0f9e5c7/268bef38/1a04e33f) deliberately settled the expectation on the resolver's real fail-closed contract. - No v2→legacy fallback. Production paths in rpc-proxy/mcp-host/OAuth were verified clean by two prior independent reviews and re-confirmed here; the interop test asserts
verifyRpcToken(v2Token) === null, andauthorizeActionV2maps any invalid/missing authority toauthority_unavailable(503) with no alternate deny (actionAuthorityV2.ts:189-206). - Control API → rpc-proxy → mcp-host propagation carries the exact access-path provenance (
accessPathId,authorizationRevision,behaviorBindingHash,pathKind,effectiveTeamId) through token → route binding → checkpoint request → trusted edge → runtime effect.
3. Non-blocking items PR3 should be aware of (test completeness, not contract defects)
None of these block PR2 as the PR3 base — the production code is correct and fail-closed. But because PR3 builds directly on this contract, I'd close the first two early:
- The genuine team-derived authorization + revocation proof runs only on real Postgres, and that lane is skip-gated.
accessCatalogFamilies.realPostgres.integration.test.tsisdescribe.skipunlessCONTROL_API_REAL_PG_ADMIN_URLis set. If that suite isn't in the pinned critical-suite CI list (prior review's T-6 saiddb.realPostgresMigration.integration/gfsUploadSession.realPostgreswere not), the strongest security evidence for the exact behavior PR3 depends on wouldn't run per-PR. Recommend: confirm this real-PG suite executes in the PR CI lane. Fix this → - The runtime interop test stubs the authorization decision.
emitActionAuthorityCheckpointV2Fixture.tscalls the realcheckpointActionAuthoritywrapper but stubsauthorize → allowedwith synthetic['fixture.authorized']capabilities, so the runtime edge never exercises a denied team-derived checkpoint. Thedenied → access_path_stalebranch (mcp-host/src/runtime/actionAuthority.ts:104-106) is not exercised with team provenance. Recommend: add a runtime-edge test that drives a revoked/denied team-derived checkpoint throughauthorizeActionV2→executeRuntimeEffect. - N1 rewrite is covered in halves. The rewrite-to-
hostis asserted with a mocked resolver (actionAuthorizer.test.ts:91-172) and the real host-authority store is exercised only assertinghost.read; no single real-resolver test asserts host authority yields aruntime_session/chatcapability. Fail-closed, but a gap worth closing when runtime v2 activates. - CodeQL model narrowness (prior CQ-F1/F3/F5): the repository-owned analyzer suppressions match by callee name / line order rather than dominance, and a couple of "unsafe" fixtures use concatenation the model rejects. Production code is safe today; these are query-hardening items, not a detection hole I could turn into a concrete exploit at this head.
Scope notes
Consistent with your framing: PR2 remains stacked on PR1, PR1 is not merged to dev, and the four-PR fold order is unchanged — I'm not recommending any merge/fold. The dormant-until-activation caveat still holds (v2 runtime effects only engage once Control API mints an actionContextV2), which is why the items in §3 are pre-activation hardening rather than live risks. I did not review PR3 implementation itself; I only checked that the PR2 contracts PR3 names are present and stable, and they are.
Overall: no must-fix blocker introduced by this PR; 4b8147a8 is safe as the technical base for PR3, with the §3 test-hardening items (especially #1 and #2) recommended to close early since PR3 depends on exactly those paths.
|
@alfredolopez80 thanks We’ve completed the full adjudication/correction cycle and synchronized the latest accepted PR1 base into PR2. Could you please do one final review of the current exact PR2 head:
Current exact-head validation is green:
Since your previous review, we also completed the verified PR2 corrections and cleaned the relevant pre-existing/base issues in PR1 before synchronizing it back into PR2. Please treat the previous findings/comments as historical unless they still reproduce on this exact current head. For this final pass, please focus on:
If a previous item still applies, please reopen it with current-head evidence. Also please distinguish any PR2-owned issue from a base/pre-existing concern. The main question is whether this exact PR2 SHA is safe to use as the PR3 stack base. |
|
@josue-tech-94 — final re-review of the exact head you asked for, following up on the previous re-review. Head reviewed: Answer to the main questionYes —
No Blocker or Critical. No regression from the PR1-base merge: I compared the committed merge with Counts, still open at this head: Blocker 0 · Critical 0 · High 1 (dormant, PR2) · Medium 3 · Low 12 1. Verdicts on items from the previous review
2. New defect introduced by the correctionsDelta-2 regression — Medium · REPRODUCED · BASE (
|
| Suite | Result |
|---|---|
control-api unit (vitest run --no-file-parallelism) |
EXIT=0 — 475 files passed / 56 skipped; 5588 passed, 0 failed, 313 skipped. The 313 skipped are exactly the real-PG tests, run next. |
control-api real Postgres (postgres:16-alpine, CI image, CONTROL_API_REAL_PG_REQUIRED=1) |
EXIT=0 — 56 files, 313 passed, 0 skipped |
control-api accessDatabaseCancellation.realPostgres under lc_messages=es_ES and C (PG15) |
EXIT=0, 10/10 under each |
rpc-proxy full suite (clean npm ci + producer deps), 6 runs |
6/6 EXIT=0, 602/602 each |
| rpc-proxy targeted (desktopProxy, sandboxUi, sandboxUi.oauthV2) | EXIT=0, 101/101 |
mcp-host (src/agent src/core/conversation src/__tests__/ironclaw src/db, Node 24) |
EXIT=0 — 64 files, 606 passed, 0 failed, 1 todo |
workflow-recipes workflowActionCheckpointClient.test.ts |
EXIT=0, 5/5 |
identity-lifecycle.test.mjs via run-node-test-files.mjs |
EXIT=0, 18/18 |
test-images-manifest.sh |
EXIT=0, 25 PASS / 0 FAIL |
test-dockerfile-local-packages.sh |
EXIT=0 baseline; 9/9 prior mutations killed; 2 new-shape mutations survive (§1 INF-3) |
5. Stack position
PR1 #334 is still open and not merged to dev; PR2 remains stacked on PR1. This review answers only whether the PR2 SHA is a safe technical base for PR3. It does not authorize merging PR1 or PR2.
Read-only review. Each mutation ran in an isolated worktree and was restored byte-exactly, and every worktree was confirmed clean afterwards.
josue-tech-94
changed the title
josue-tech-94
changed the title
We’ve completed the remaining correction and stack-convergence work, including the final PR1 CI follow-up. Could you please do one final review? Current validation is green: The previously identified correction, migration, image/build-guard, lifecycle-test, package-resolution, and synchronization issues have been addressed and revalidated. Please treat earlier review findings as historical unless they still reproduce on this exact current head. For this final pass, please focus on:
If an earlier finding is still applicable, please reopen it with current-head evidence, and distinguish PR2-owned issues from base/pre-existing concerns. Thanks. |
|
@josue-tech-94 Re-review at the exact head Per your request, earlier findings are treated as historical unless they reproduce on this head, and every item is tagged PR2-OWNED or BASE/INHERITED. Verdict on your last question: is this SHA safe as the PR3 technical base?Not yet — two items to land first, both small. This is a change from my round-3 answer, and neither cause is a defect you introduced in the v2 logic.
Everything else is follow-up or Low. The v2 authority core itself held up well: the gfs-controller checkpoint is fail-closed, the upload-session authority binding cannot be downgraded mid-flight, and the contracts package has no version skew across its four consumers. Scope note: merge-base(PR1, PR2) equals PR1's head Blocker — v2 delegation returns 400 on every body-bound routeDormant · certain when enabled · root cause BASE, one half PR2-OWNED
router.post('/rpc/:serverName',
requireRpcAuth,
requireScope('mcp:server:invoke'), // v2 binding resolves here
jsonBody, // body parsed after
Affected: Root cause is inherited: #669 / Why CI is green: 15 rpc-proxy test files install their own body parser; only 3 exercise the real This is fail-closed, not a bypass — no security exposure. But v2 is unusable until it is fixed. Fix: move HighH1 —
|
| # | Finding | Attribution |
|---|---|---|
| M1 | The six PR2 migrations renumbered 010f→0115 … 0114→011a, none carrying legacyVersions. The only alias guard (db.userAccessFoundationMigration.test.ts:35) pins PR1's 0109-010e only. Lower risk than H1 because those names never reached main |
PR2-OWNED |
| M2 | migrationExecutionPolicy.realPostgres.integration.test.ts:362 — the test named "converges accepted legacy aliases without replaying historical bodies" is vacuous: removing an alias leaves it green (EXIT=0, 10/10). It never asserts migration.apply was skipped. This is why H1 and M1 went undetected |
BASE |
| M3 | The real-Postgres suite-list guard is one-directional: it validates listed suites but never requires every *.realPostgres.integration.test.ts to be listed. 37 pinned, 62 on disk. EXP5a: removing accessCatalogFamilies… → EXIT=0. Mitigating: the aggregate --run realPostgres still executes them; what is lost is the rename tripwire |
PR2-OWNED (e2c76bcf4) |
| M4 | make install-all (Makefile:112-117) uses bare wait, which returns 0 unconditionally. make install-all SERVICES=/nonexistent-service-xyz → EXIT=0, "All installs complete." validate-all runs it immediately before test-unit-all, so M4 and H4 compose: a whole-repo install failure followed by a zero-test "pass" |
BASE (5d663a496) |
| M5 | The R6-M4 comment at test-dockerfile-local-packages.sh:518-521 claims omission of a published consumer "cannot leave Docker materialization or change detection green together". EXP3 removed grok-llm-proxy from images.json and all 10 of its build-publish.yml lines → that guard EXIT=0. The real population oracle is scripts/minikube/build-images.sh, a third file. The independence you added is real but narrower than the comment states |
PR2-OWNED (52f6a5185) |
| M6 | sandboxUi.ts:876 — the res.on('close', cleanup) mutant survives 3 runs |
PR2-OWNED |
| M7 | DELETE …/oauth/grant middleware order is untested; 8f46d9818 claims 2 routes and covers 1 |
PR2-OWNED |
Low
db.ts:3213-3216comments assert migrations apply "strictly in array order". False since the sync: 18 of 21 post-0106migrations execute at a different index. PR2-OWNED.- Prefix
0109is shared by0109_grok_subscription_connectionsand0109_user_access_foundation. No behavioural consequence — every consumer keys on the full string; noslice(0,4), noparseInt, nosortanywhere.db.migrationsOrder.test.ts:28-32does require strictly increasing order and passes on full-string comparison. What is missing is any assertion of prefix uniqueness. BASE. migrationRunner.ts:108-118— before-schema preparation is guarded by!hasPostSchemaIndexes, so a migration declaring both phases would skip its before-schema indexes. Latent; no such migration exists today. PR2-OWNED.migrationExecutionPolicy.test.ts:58-60was weakened to…startsWith('0115')).toHaveLength(1)by981062749; the title says "exactly 25" while the registry has 26. PR2-OWNED.seed.mjs:19-21— two imports run beforetry {. If either throws, the catch never runs and Node prints the raw error. EXP6c (stubbing/app/control-api/dist/db.jsto throw a DSN-bearing error) leaked the DSN. Whether that module can actually throw such an error at import time is unverified, so this is latent; thepoolimport predates6f10dfa69.identity-lifecycle.test.mjs:43-52is a source-text positional assertion (indexOfon three literals plus ordering). It fails loud if the binding moves — not vacuous — but proves nothing about runtime behaviour and would pass if the catch emitted the full message. Consider adding a behavioural case that drives the catch path. PR2-OWNED.sandboxUi.ts:~828per-requestproxyResfilter untested; two restored v2 branches (:405,:322-326) untested; malformed comment at:796-800. PR2-OWNED.- D3:
actionDestination.ts:50-52returns null forchat, soactionAuthorityV2.ts:150-155→ 400invalid_binding. No consumer ofchat.readexists yet, so it is latent for PR3. - D4:
gfsReadiness…:157still matches on error text, and was widened this round (ea2375678) with two migration-name fragments rather than moving to SQLSTATE42501. - D2 residual: both branches end in 503
authority_unavailablewithretryable:true. No authorization consequence. - CQ-F6:
.github/workflows/codeql.yml:21limitspull_requestto[main, dev], so CodeQL does not run on this PR's base branch. build_publish_matrix_rows()attest-dockerfile-local-packages.sh:275computes an unusedpublishedvariable — dead code that reads as if the manifest were being consulted.- The PR body is stale. It cites head
8f46d9818and "Manual exact-head CodeQL workflow 35399907982 succeeded for this exact SHA". No PR2-owned commit landed after8f46d9818, but 87 files / +7556/−1728 entered via inherited merges, so the CodeQL and "collateral CLEAN" evidence describes a tree that no longer exists.
Verified clean
@clerum/action-context-contractsis linked byfile:from all four consumers — no version skew.gfs-controller/src/api/serve.ts:272-337checkpointRouteAuthorityis fail-closed with an exhaustive else-throw.gfs-controller/src/upload/uploadSession.ts:321-330assertUploadAuthorityOriginprevents a v2-opened session from being downgraded to v1 mid-flight.- No reference anywhere in the tree to a migration id absent from the registry.
- Release coordinates:
rpc-proxy0.1.99 = lockfile (both fields) =releaseManifest.rpcProxyVersion;external-rest-api0.1.109 consistent;desktop-app0.8.1 =desktopVersion;releaseId v0.8.1,minimumDesktopVersion 0.1.252. 6f666f047Grok dependency is coherent acrosspackage.json, both lockfile fields, thesource_pathsof all four consuming images, and the Dockerfile COPY-before-npm cirule.- The
ci-public.ymlreal-Postgres block opens withset -euo pipefail, so its| teeis not masked. Nocontinue-on-errorin the file; the only two|| trueare benign. The block requires bothTest Files N passedandTests N passedand rejects anyskipped. - The suite-list guard has a genuine self-mutation test at
:61-75— mutates a copy, re-runs itself, requires failure. That is the right shape. - 30 published images in
deploy/images.json= 30build-publish.ymlmatrix rows.
Suites
| Suite | EXIT | Result |
|---|---|---|
| control-api unit | 0 | 497 files, 6007 passed, 0 failed, 351 skipped |
| control-api real Postgres 16 | 0 | 63 files, 351 passed |
| rpc-proxy | 0 (5 runs) | 617/617; 1 run in 6 EXIT=1 (pre-existing flake) |
| mcp-host | 0 | 71 files, 691 passed |
| workflow-recipes | 0 | 147 files, 2889 passed |
test-dockerfile-local-packages.sh |
0 | PASS |
test-images-manifest.sh |
0 | 25 PASS / 0 FAIL |
test-release-coordinates.sh |
0 | 34 PASS / 0 FAIL |
test-ci-public-real-postgres-suite-list.sh |
0 | PASS |
node --test identity-lifecycle.test.mjs |
0 | 19/19 |
| PR #594 CI | — | 40/40 pass (CodeQL absent on this base — see CQ-F6) |
| PR #334 CI | — | 41/41 pass (includes CodeQL run 35616468291) |
Suggested order
- Blocker —
jsonBodyordering, plusjsonBodyon/wake, plus onecreateApp()integration test on a v2 body-bound route. - H1 — add the
legacyVersions: ['0115_llm_allowed_models_image_input']alias. - M2 — make the alias convergence test non-vacuous; it is the guard that would have caught H1 and M1 for free.
- Then this SHA is a sound technical base for PR3. H2/H3/H4 and the rest are real but belong in their own issues — they are CI-guard coverage, not runtime correctness, and two of the three predate this PR.
Read-only review: no commits, pushes, branches, Docker changes or image pulls. git status --porcelain was empty at the end of every agent; the one accidental package-lock.json rewrite (from an experimental make install-all) was detected and restored from HEAD.
🤖 Generated with Claude Code
e56fc39 to
b311ff9
Compare
The only conflict was the workflow-recipes version line in package.json and package-lock.json, resolved to the branch side (0.1.499); the version hook then bumped it to 0.1.500. The removal of the dead lint script from dev is kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#1135 bounds status reports in workflow-runtime-core and workflow-recipes. Conflicts: - packages/workflow-runtime-core/src/status-reporter/client.ts: dev now reads every error answer once as text (responseErrorReason, cause code, bodyBytes). The branch's 423 pause and 503 run-store checks read the body again with resp.json(), which would fail on a consumed body and silently stop detecting both codes. They now parse the same text (responseErrorCode). The final error keeps dev's cause detail and the branch's WorkflowRunStateUnavailableError. - tests/unit/status-reporter.test.ts: dev's file plus the branch's run-store describe, whose mocks now answer text(). Two tests pin the 423 pause detection through the reporter, which had no unit test; reverting the client to the double body read fails the first one. - workflow-recipes version: resolved to the branch's 0.1.500 (dev 0.1.498); the version hook then set 0.1.501. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… with an authority binding
A V1 `running` report no longer reads the run store. Before this change
every workflow-level and step-level `running` report of a DB-run child read
`workflow_runs` (joined with `workflow_authority_bindings`) to learn whether
the run was bound, so a V1 run answered 503 while the store was unreachable
and threw on any other database error. On dev these routes do not read
Postgres.
- crdConstants: add RUN_AUTHORITY_LABEL (`clerum.io/workflow-run-authority`)
and RUN_AUTHORITY_BOUND (`bound`).
- dbRunChildRecipeCreator: stamp the label on the child when the run row
carries an authority binding. The binding is fixed when the row is
inserted, so the label is decided once. On a 409, an existing child whose
authority label differs from the expected one is rejected like any other
identity mismatch.
- restEndpoints: checkpointWorkflowRunBoundary runs the durable boundary only
for a child whose label equals `bound`; any other value or no label is a V1
child decided by recipe status, as on dev. `durablyAllowedRunning` (the
pause-projection clear) uses the same predicate.
- A labelled child whose run has no row still gets 409; an unlabelled child
with no row is accepted, as on dev.
- dbRunProcessor.ts: comment-only edit of the V1 branch comment above
`if (!observed.authority_binding) return { outcome: 'allowed' }`. That line
is still reachable through restEndpoints only for a labelled child whose run
has no binding, which workflow-recipes never creates. Its test is kept.
- The status GET still reads the run for V1 children and can overlay and
re-publish a pause for an unlabelled child with a bound row; no such child
exists at this head (gate for enabling V2).
- docs/deploy/releases-and-upgrades.md: the gfs-controller sentence no longer
names a release; add the workflow-recipes rollout order against migrations
0139 and 0143.
Tests: new wiring test from the real child creator to the real handlers and
run processor over a real pool on a closed port (V1 child: 200 and no client
requested; bound child: 503, or a throw on 42P01); creator label tests; the
fake-processor fixture carries the label; the run-store wire test is split
into a bound run (three 503s, one wait) and a V1 run (no client requested).
control-api's real-Postgres trigger test gives its hand-built bound child the
label; it runs only in the T1 real-Postgres lane and was not executed here.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the direct-service limit fix(mcp-host): do not charge timer-driven reads or the progress stream against the direct-service limit channel-reader polls the cron results, the workflow-approval notification claim and every in-flight task result about every 2 s. All of them were charged against the single 600-per-minute direct-service bucket, so with about 18 in-flight tasks the polls alone filled it and new messages, approvals and decisions got 429 until the window ended. The progress stream of a task the Host had already accepted was charged a second time, so a Telegram reply could be lost when the bucket filled in between. Stop calling admitDirectServiceRequest on six routes: - GET /v1/runtime/cron/results and DELETE /v1/runtime/cron/results/:taskId - POST /v1/runtime/workflow-approval-notifications/claim and POST .../deliveries/:id/ack|fail - GET /v1/runtime/tasks/:taskId/result - GET /v1/runtime/tasks/:taskId/progress/stream The bucket keeps the eight routes that start work: messages, tool approval (approve and deny), provider message authorization, workflow approval decide, resolve and result request, approval-medium enrollment and Telegram verification. The function and class comments now list what is charged; the function comment no longer cites the Spec 62/65/48 boundaries and states that RPC Proxy ingress is not charged here. Tests: a real Host with a recording handler on all fourteen routes and the real channel-reader clients: 900 polls leave the action budget untouched; at a full bucket the six exempt routes (including the real progress stream client) are served and the bucket stays full; a table over the routes checks 429, Retry-After and Cache-Control on the charged ones, with the same request served after the window rolls over. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the Host no longer limits The MCP Host no longer charges the cron-result poll and its acknowledgement, the notification claim and its ack/fail, the task-result read or the progress stream against its direct-service limit, so it never answers 429 on them. The handling channel-reader had for that answer lost its only producer (admitDirectServiceRequest). rpcClient.ts: - remove RUNTIME_ADMISSION_RETRY_AFTER_SECONDS_DEFAULT, RuntimeServiceAdmissionLimitedError, runtimeAdmissionRetryAfter and the notificationAdmissionRetryUntil / cronResultsAdmissionRetryUntil fields; - fetchDeliveries, getCronResults and acknowledgeCronResult lose their Retry-After pre-checks; fetchDeliveries, getTaskResult, getCronResults and acknowledgeCronResult lose their 429 branches, so a 429 there takes the generic error path; - getCronResults' catch returns [] for every error again, as on dev. main.ts: drop the RuntimeServiceAdmissionLimitedError import and restore dev's result-polling fallback loop (plain 2 s sleep; no Retry-After delay or deadline clamp). Kept: RUNTIME_ADMISSION_RETRY_AFTER_SECONDS_MAX, runtimeAdmissionRetryAfterFromHeader, directServiceAdmissionRetryAfter and the 429 handling of the message POST and provider-message authorization, which stay charged. The other charged calls (approve, deny, decide, resolve, result request, Telegram challenge confirm, both link-session confirms) had no 429 handling before this commit either. postNotificationDeliveryTerminal had no 429 branch, so there was none to remove for ack/fail. The comment above the Telegram progress-flow catch in main.ts now says why that catch is reached only by the status message send. Tests: the four rpcClient tests that pinned the removed 429 handling and the class import are removed. orphans checked: RUNTIME_ADMISSION_RETRY_AFTER_SECONDS_DEFAULT, RuntimeServiceAdmissionLimitedError, runtimeAdmissionRetryAfter, notificationAdmissionRetryUntil, cronResultsAdmissionRetryUntil, the pre-checks and 429 branches of fetchDeliveries, getTaskResult, getCronResults and acknowledgeCronResult, the getCronResults rethrow, retryDelayMs and the deadline clamp in main.ts, RUNTIME_ADMISSION_RETRY_AFTER_SECONDS_MAX, runtimeAdmissionRetryAfterFromHeader, directServiceAdmissionRetryAfter; removed: all but the last three, which the message POST and provider-message authorization still use. No other package references the removed symbols. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t, retry Slack/Teams once
fix(workflow-approval-request-reader): honour the Host admission limit and retry Slack and Teams decisions once
The reader treated the MCP Host's direct-service admission refusal like any
other failure: it read neither Retry-After nor retryAfterSeconds, told Slack
and Teams users "Try again in a moment" after the webhook had already been
acknowledged (so the provider never presents the click again), and
forwarded 429 without Retry-After on the synchronous paths.
- mcpHostClient: a Host answer is the admission refusal only when the
status is 429, the body error is runtime_service_admission_limited and
Retry-After is an integer from 1 to 60 equal to the body's
retryAfterSeconds (same check as channel-reader). Decision and enrollment
then return { ok: false, status: 429, error, retryAfterSeconds } and log
it at warn. Any other 429 keeps the generic result.
- Synchronous decision (Telegram and generic callers) and enrollment: the
response carries Retry-After; the body and status are unchanged; no retry.
- New decisionAdmissionRetry module: one process-wide queue, at most one
armed retry per approval request id (trimmed, lower-cased), at most 64,
one setTimeout of retryAfterSeconds each; the entry is removed before its
runner runs; close() refuses later schedules.
- Slack and Teams background decisions: a canonical 429 on the first
attempt arms one retry and tells the user it will be sent again in N
seconds. The retry re-submits the same command once (same
providerEventId; route resolution and the can-approve check run again);
its result goes through the existing feedback, and a second refusal says
"The assistant is busy. Try again in N seconds." without arming another.
When the queue is full or closed, or another click armed it, the user
gets that same text. A click on an approval whose retry is armed is not
submitted: the same user's same decision is only logged; another
decision gets a Slack thread message (Teams: log only).
- main.ts: shutdown first closes the retry queue and logs
cancelledDecisionRetries. Pending retries are in memory only and are lost
when the reader restarts.
Tests: queue unit tests with fake timers; server scenarios for retry
success, a second refusal, a stale approval, held clicks, two concurrent
refusals, a full queue, Teams, no feedback context, the synchronous path
and a malformed 429; a shutdown test; client tests for the canonical and
malformed answers; and in mcp-host's admission test, the real Host with a
full bucket gives the real reader client the wait, and the same command
reaches the handler after the window rolls over.
The reader is not covered by validate-release-version-bumps.mjs and the
version hook does not list it, so its version is unchanged; mcp-host is
bumped for its test file.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…omments docs: correct the comments of T-1 and T-9 (control-api, comment-only) Comment-only; no behaviour change. - oauth/tokenHelper.ts: the check at the top of getAccessToken does not stop every cached token. Its callers are the refresh path of getAccessTokenReactive (a stale token with a refresh token) and the proactive refresh sweep; the reactive fast path returns a token that is not stale without reading the recipe (reactiveTokenHelper.ts), so this check does not cover that token. - services/hostRpcAdmission.ts: admitHostRpc is called only by the V2 action checkpoint route (routes/internal/actionAuthorityCheckpoint.ts); legacy Host routes are not charged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…le a retry is armed The Host charges its admission limit on the decide route before it authorizes the decider, and the reader's control-api can-approve consult runs only for commands with a channel alias. A user without approval rights could therefore receive the admission 429 and arm the per-approval retry; every other user's click (or the other decision) was then held for up to 60 s with "Another decision for this workflow approval is waiting to be sent", and the armed retry was later denied. While a retry is armed, only a repeat of the armed click (same provider user, same decision) is held, and only logged. A click from another user, or the other decision, is submitted at once through the normal background path and gets its own feedback. The armed retry stays; if the approval is decided meanwhile, the retry shows the existing "no longer pending" text. The "Another decision ... is waiting" text and its code path are removed. Tests: the other-user scenario now expects an immediate submit with its own result, and the armed retry running once afterwards; a new scenario covers the same user's other decision. orphans checked: ANOTHER_DECISION_PENDING_TEXT, holdSlackDecisionClick, armedDecisionRetry, the send_held feedback action; removed: all of them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onous enrollment 429 The synchronous enrollment path forwards the Host's admission wait as Retry-After (b4566e3), but no test covered it: removing the header line left every test green. The new test drives a Telegram /start enrollment whose submit is refused with the canonical 429 and asserts 429, the Retry-After header and the body, and that nothing is retried after 60 s. Removing the header line now fails it (expected null to be '9'). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e status read The status read comment said every re-sent `running` report is re-gated by the durable boundary. Since 6fa26f1 the boundary runs only for children carrying the run-authority label; workflow-recipes labels every child of a run with an authority binding, so an unlabelled child (none exists while V2 cannot be enabled) is not re-gated. Comment only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hile its retry is armed A decision refused by the Host limit is retried once in the background. Until now (87b242c) only a repeated click by the same user with the same decision was held while that retry was armed; the same user's other decision was submitted at once. If that click was refused as well, the armed retry later sent the earlier decision after the user had chosen differently; if it was accepted, the second click won, unlike the unsaturated path where the first click decides. While a user's retry is armed, every later click of that user is now held (log only; on Slack the other decision gets a thread message saying the earlier decision is still being sent). The approval stays pending for its requester until that retry ends; if it is refused again nothing stays armed and the user can choose again. Clicks of other users are still submitted at once: control-api accepts a decision only from the approval's requester, so they do not change it. Tests: the same user's other decision is held and the first decision is sent after the wait; another user's click is submitted, refused with approval_requester_mismatch, and the armed retry still sends the requester's decision. Mutations: holding only the same decision fails the first; holding nobody fails the duplicate and first tests; holding every user fails the second. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#1339 lets HCC's network-policy pass settle. The only conflicts were the version lines of host-context-controller (dev's 0.1.445 is higher than the branch's 0.1.427; set to 0.1.446 because this branch changes HCC code) and mcp-host (the branch's 0.1.662; the version hook then set 0.1.663). host-context-controller/src/hostReconciler.ts and both config.ts files merged without a conflict; HCC build and suite (129 files, 3115 tests) and mcp-host typecheck and suite (618 files, 9149 tests) pass on the merge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the deep pass on Must resolve
V1 behaviour changes
V2 path (V2 cannot be enabled at this head): R1-M7 to R1-M14 are confirmed and kept as gates for enabling V2 (PR3). For R1-M10 the description now says the 409 pauses the run durably. Low: R1-L4 → #1478, which now covers seven routes (the sandbox-ui authorize-url route too). R1-L5 and R1-L20 are declared. R1-L6 is confirmed (V2 gate). The R1-L12 comment in |
|
Took another deep pass, this time on 🔴 Not ready to approve
This is my read from the review, not a formal GH approval. One structural note before the items. Must resolve before approval
Optional (non-blocking)These can merge as follow-ups or at your discretion. The exception is the seven Mediums in the first group: they've been open two rounds now, so each one either gets a written V2-gate decision or a fix. They shouldn't float into round 3. Carried Mediums that need a decision (all still open since round 1)
New this round
Low still open since round 1No new evidence on these; the code is unchanged unless noted. Details are in last round's comment.
Closed since round 1: R1-B1, R1-M1, R1-M2, R1-M3, R1-M4 are fixed. R1-M5, R1-M6, R1-M10, R1-L2, R1-L4, R1-L5 and R1-L20 are now declared in the body, so I'm treating them as accepted. What checked out clean
|
… program pre-gate-sync.sh reads the HCC Host edge-protocol selector with a jq program in single quotes. 574bbb8 ended two of its lines with a backslash; inside single quotes the shell passes it to jq unchanged, and jq rejects it ("unexpected INVALID_CHARACTER", exit 3, on jq 1.7.1 and 1.8.1). Every pre-gate-sync run that reached the rollout block stopped there, so T2 failed in minikube-pre-gate-sync after a full bootstrap. jq accepts newlines, so the two backslashes are removed. Checked against the host-context-controller Deployment of a freshly bootstrapped profile: the old program fails to compile on both jq builds; the new one prints "legacy-headers" and still fails with "protocol selector must be explicit" for zero or two selector entries. A sweep of every shell script and makefile found no other jq program with a line continuation inside its quotes. shellcheck findings are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#1453 makes the workflow runtime wait for the snippet runner before it reports the run as running. In sdkRuntime.ts that wait sat where this branch replaced the plain 'running' report with reportRunningBoundary, so both are kept in that order: the runner readiness wait first (only when a pending step uses the runner), then the running boundary. The deps interface and the error classes keep both sides. The branch's wire test for the running boundary built a snippet-runner fake without healthCheck, which the merged runtime now calls; the fake gets the same healthCheck that #1453 gave its own fakes. workflow-recipes is set to 0.1.506, above dev's 0.1.505, because this branch changes its code. Its build and suite (159 files, 3494 tests) pass on the merge, and the release version validator accepts it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The workflow approval reader can only replace a Teams approval card
(update-message PUTs the activity with `attachments: []`), so every
per-user notice removed the card's buttons. Add
POST /internal/workflow-approval-reader/teams-targets/:targetId/send-message
so the reader can post a new plain-text message instead.
- Same caller identity, target resolution and refusals as update-message:
requireInternalToken + requireInternalService('workflow-approval-reader'),
resolveTeamsTarget, the serviceUrl allowlist checked before the Secret is
read, the same app password and token exchange.
- The Secret read and token exchange move unchanged into
teamsBotAccessTokenForTarget, used by both routes; update-message keeps its
status codes and bodies (its existing tests pass unmodified, and new tests
pin its refusal rows).
- Threading is decided here from the CommunicationChannel: the group with
channelId === conversationId and the target's tenant; only
`replyInThreads: false` posts at the top level of the conversation, as
channel-reader does. Threaded and a card id given: POST
.../conversations/{threadConversationId || conversationId}/activities/{replyToId}
with { type, text, replyToId }; otherwise POST
.../conversations/{conversationId}/activities with { type, text }. Every
path segment is percent-encoded. The activity is built from text and
replyToId only; caller attachments, entities or threading flags are ignored.
- Upstream non-2xx -> 502 teams_send_failed with providerError; the text is
never logged.
No RBAC, NetworkPolicy, CRD or deployment change.
Version: control-api 0.1.963 -> 0.1.964 (pre-commit hook).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nly when recorded or stale
Change the approval message only when a decision is recorded or it is no
longer pending.
The Slack and Teams result handlers ran the first-attempt feedback of dev for
the background retry too. A retry arrives up to 60 s after the click, after
other actors may have decided, and its stale answer rewrote the shared
approval message (Slack and Teams) with a relaunch text. In Teams every
notice was delivered by replacing the card, which removes its buttons while
the approval is still pending.
- New pure module decisionFeedback.ts: planDecisionFeedback maps (medium,
decision, attempt, result, arming) to { shared, notice }. A recorded
decision rewrites the shared message; a first attempt that finds the
approval no longer pending marks it stale; a retry changes it only when it
succeeds. Stale is keyed by the error code (approval_not_pending,
approval_route_not_found) whatever the status: 409 from the Host, 403 from
the can-approve consult. Every other result is a notice. The feedback
texts move there; server.ts keeps none.
- The first-attempt stale text no longer asks to trigger the workflow again.
A retry that finds the approval decided sends "Your earlier decision was
not recorded: this workflow approval is no longer pending." The held-click
text says the result appears on the approval message, or in a message if
it is not recorded.
- Teams notices are new plain-text messages through control-api's
teams-targets/:targetId/send-message route (sendTeamsTargetMessage); the
card is replaced only on a recorded decision or a first-attempt stale
answer, and a held click of the other decision now gets the held notice.
The reader passes the card's ids (conversationId, the payload's
conversation.id, replyToId); control-api places the message from the
group's reply-in-threads setting.
- Slack notices (including the failure notice inherited from dev) follow the
target's slackSettings.replyInThreads as verified by control-api: in the
approval message's thread when true, at the top level of the channel
otherwise.
- What is sent to the Host and control-api is unchanged: arming, holding and
the retry runner keep their logic.
- Docstrings of holdClickWhileOwnRetryPending and armDecisionAdmissionRetry
state the hold only once a retry is armed, and that control-api decides who
may decide (target user; for a team approval the recorded requester, or
any active team member with the team's workflow trigger permission when
none was recorded). README updated.
Tests: decisionFeedback.test.ts (table per row and medium, 403 and 409 stale
forms, invariant over every retry input with an ok-sweep witness), Teams and
Slack server cases for each changed row, notice placement for
replyInThreads true / false / unset and a threaded approval message, the
one-user race (first click armed), and the client call.
Version: the reader is not a pre-commit bump root (stays 0.1.0); no
package.json version changes in this commit.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…al from the real Host
The reader's canonical 429 case built the Host's answer by hand, so a change
to the Host's body or header would leave it green, and the enrollment route
had no test against the producer anywhere.
mcpHostClient.hostAdmission.integration.test.ts boots the real MCP Host
(`../../mcp-host/src/server`, which also makes CI install mcp-host's
dependencies for this package), fills its direct service admission bucket
(600 admits, the 601st refused as a witness) and asserts that
submitMcpHostDecision and submitMcpHostEnrollment each read
{ ok: false, status: 429, error: 'runtime_service_admission_limited',
retryAfterSeconds: 1..60 } with no fetch stub, and that neither handler ran;
once the window rolls over the same calls reach each handler once.
The hand-built canonical case of mcpHostClient.test.ts is removed; the
malformed 429 cases stay hand-built, since no producer emits them.
Mutations: removing the 429 branch of either client call, or renaming the
Host's `retryAfterSeconds` body field, fails the new test while
mcpHostClient.test.ts stays green.
Version: test-only change in the reader, which is not a pre-commit bump
root; no package.json version changes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…a child with run authority getWorkflowStatus read workflow_runs for any recipe with a run id and used the answer only when the row had authority. The running boundary already skips the store for a child without the run-authority label (checkpointWorkflowRunBoundary), but the GET did not: for a V1 child the read can only answer hasAuthority false or fail, and while the store is unreachable every status GET (including the runtime's startup readCurrentStatus) waited on it and logged at error. The GET now reads the store only when the recipe carries the run-authority label (recipeCarriesRunAuthority), as at the running boundary. Without it, `durable` stays null, so the phase overlay and the pause re-publish are skipped by their existing conditions; labelled children are unchanged. The claims half of the run id (`labelRunId ?? claims.runId`) is left as it is: no token that the status GET accepts carries a run id. The GET needs scope status_read or status_write; the workflow-recipes tokens with audience clerum-wrc take no run id, and control-api's delegation token, which does carry one, is scoped to artifact_read, admin:artifact_read or admin:artifact_delete only (control-api/src/utils/auth/delegationToken.ts:12). Tests: a child without the run-authority label and a store that refuses connections answers 200 from the recipe status with no read and no error log; T-D1f (no labels, run id in the claims) now expects no read; the V1 wire case asserts the runtime's status reads answer 200 without a pool query. Mutations: removing the condition fails all three; giving the `authority: false` fixture the label fails the new test. Version: workflow-recipes 0.1.506 -> 0.1.507 (pre-commit hook). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…om authority answers
control-api's V2 wake route uses 404 and 409 both for an authority refusal
({ version: 2, status: 'not_found' | 'access_path_stale', code }) and for
the wake result after the authority check passed ({ status: 'unknown' } on
404, { status: 'not-stateless' } on 409). rpc-proxy read every V2 404/409
as an authority answer and threw "invalid authority response" for the two
wake results, so the wake route answered 502 and wake-and-hold treated them
as an unreachable endpoint (or kept holding on a retrigger).
After the single body read of the V2 block, the pair (HTTP status, body
status) decides: 404 with `unknown` returns { kind: 'unknown' }, 409 with
`not-stateless` returns { kind: 'not-stateless' }, as the V1 path already
does. A crossed pair (unknown on 409, not-stateless on 404) or a non-JSON
body still throws, and authority answers are unchanged. The HTTP status
alone cannot decide: an authority access_path_stale is also a 409.
The two wake bodies live in control-api/test/fixtures/hostWakeV2Answers.ts,
pinned against the real route in actionAuthorityHostWake.test.ts and
imported by rpc-proxy's new host-wake-v2-answers.test.ts (rows c1-c9:
wake answers, authority 404/409/403, crossed pairs, non-JSON, V1 path).
Mutations: dropping the two lines fails c1/c2; dropping the HTTP half
fails c5/c6; deciding on the HTTP status alone fails c3/c4.
Callers with V2 on: the wake route answers 404/409 instead of 502;
wake-and-hold settles not-found (404) or legacy not-stateless, including
for the waiters of a mid-hold retrigger, as the V1 path does.
Version: rpc-proxy 0.1.142 -> 0.1.143 and control-api 0.1.964 -> 0.1.965
(pre-commit hook; the control-api change is test-only), with rpcProxyVersion
re-synced in external-rest-api/src/releaseManifest.ts.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
workflow_runs.run_id is the uuid primary key, but seven predicates compared `run_id::text = $n`, so Postgres could not use workflow_runs_pkey: the resolver's resume pre-read, LOCK_RUN_FOR_RESUME and RESUME_PAUSED_RUN, and the four run and artifact branches of the authority candidates statement. The cast existed because the value is an unvalidated string, and a text comparison cannot fail on a malformed id. canonicalWorkflowRunId (services/access/workflowRunId.ts) returns the id only when it is exactly what run_id::text renders (lowercase, hyphenated), so every verdict stays the same: - resolver pre-read: a non-canonical id returns "no run" without a query, otherwise `run_id = $1::uuid`; - resumeWorkflowRun: a non-canonical id answers 404 workflow_run_not_found before the transaction (inside the try, so the budget is still closed); both statements compare `run_id = $1::uuid`; - candidates statement: two parameters computed in JS, $6 for a workflow_run and $7 for a workflow_artifact's first segment (what split_part($3, '/', 1) yields), null otherwise, which matches no row. The artifact name condition `split_part($3, '/', 2) <> ''` stays in SQL. $3::uuid is not used: whether Postgres evaluates the cast in a branch excluded by $2 is not relied on. Tests: the helper's accepted and refused shapes; resume rejects `not-a-uuid` and an uppercase id with 404 and no statement, and its statements carry `$1::uuid`; the resolver skips the pre-read for a non-canonical id; the artifact SQL assertion follows `$7::uuid`. On real PostgreSQL, the candidates statement gives the same grants as before for canonical, missing, uppercase, braced, unhyphenated, padded and malformed run ids, artifact ids (<id>/<name>, <id>/a/b, <id>/, <id>//x, malformed first segment) and the approval, GFS and notification branches, and a resume with a malformed id answers 404 instead of a database error. EXPLAIN on PostgreSQL 16 shows every changed statement using workflow_runs_pkey after the change and none before. Version: control-api 0.1.965 -> 0.1.966 (pre-commit hook). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…althCheck The Real PostgreSQL revocation test in workflowTriggerSelectedPath.realPostgres.integration.test.ts drives workflow-recipes' runWorkflowRuntime with a snippet-runner fake that had only execute. Since dev's #1453, merged into this branch in 19f88d7, the runtime calls snippetRunner.healthCheck() before a pending snippet step, so the test failed with "client.healthCheck is not a function". The merge gave the workflow-recipes wire fixture the same method; this fixture was missed. The real SnippetRunnerClient has healthCheck (workflow-recipes/src/workflow/snippetRunnerClient.ts:57). With this line the file passes 14/14 on a local PostgreSQL 16 (evidence impl-r2/final/probe-healthcheck.log). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| id?: string | ||
| error?: string | ||
| } | ||
| if (!response.ok) { | ||
| res.status(502).json({ | ||
| error: 'teams_send_failed', | ||
| providerError: body.error || `http_${response.status}`, |
PR2: runtime actions use validated access paths
PR2 of the Team Token removal (Task 106). It adds server-validated, user-centric access paths for runtime actions (workflows, GFS uploads,
RPC delegations, approval decisions). V2 stays off:
actionContextV2andrpcDelegationV2readCONTROL_API_USER_ACCESS_ACTION_CONTEXT_V2andCONTROL_API_USER_ACCESS_RPC_DELEGATION_V2, defaultfalse(
control-api/src/services/access/userAccessPolicy.ts:213-214), and no manifest underdeploy/orcharts/sets them. Publishing thishead is not a merge or deployment approval.
Status
aa8e6a877, a fast-forward of the previously published heads054a382a1,b22ceb213,72f49fb99,603103d92,5a2a1333c,3f28a505fand14100a24f(and of the earlierc64989e7c,a7d282ace,378d2cdcf,0d1c00106,73fcf94c8,bd86ac5c7).devat193f5f4a7(merged inaa8e6a877). Ifdevmoves again, it is merged once more before T2.3f28a505fand5a2a1333cfailed the control-api and Real PostgreSQL jobs:3f28a505fmade gfs-controller andworkspace-files-controller import
@clerum/action-context-contractsby package name, and those jobs do not install eitherservice's dependencies.
36071cc1arestores the relative import. Two other failures on5a2a1333care in tests no commit since3f28a505ftouches, where they passed: the T2 stale-lock race test (test(minikube): T2 stale-lock race test intermittently admits two owners in CI #1254) and the egress proxy TLS contract(
startup_no_live_upstreams=1).build-argstwice since the merge
3c501d7ba, which combineddev's profile-ui argument (2d9f0fe0b) with this branch'sEVENFIRE_*arguments (286537793) in two different hunks. Merged intodevit would have stoppeddev's Build & Publish.603103d92keeps one key;actionlintpasses.72f49fb99(five lanes) and the external review of14100a24fwere triaged together; the commits after9622b5c7dfix the four findings that change V1 behaviour and the false sentences they found (see "Verification").What changed since
c64989e7cPR1 (#334) landed in
devin a reduced form: the aggregate access catalog, the operational resource index and its indexer were removed.PR2 was rebuilt on that
devby merge (no rebase, no force-push). Read the first-parent commits; the full diff againstdevalso containseverything PR2 already had.
d0c53e89bdevd02fcb9f1(PR1 #334). Deletes the access catalog files PR2 had inherited. Does not compile alone; the next commits restore it.21cc4ce5b402877017c64989e7cthe action authorization modulesdevremoved with the catalog (resolver, candidates, projection, reader, access paths, resource identity, execution budget, the two V2 flags). Of the access routes onlyGET /external/access/capabilitiesand the edgeGET /me/access/capabilitiesstay;POST /external/access/resolveand the catalog routes are gone.05e8492b20138(see "Migrations").af8fa56bb,172d2bd35fb3ed37f9,7426f8d37devdoes, instead of from the removed index; tests against a live Kubernetes gateway.84da26356,76303560d,901ff3b0e,3054f2272c84210bc82c8da993ebe36aab83pool.on('error')and a client error listener).1c98b2718,5b346af47,3654241be,4621af067,6083919e3cbe1f2fe1,d8d0af11aa2900818d,0f13db2c0dev18052d9ceande611ebfcb. Version conflicts resolved to the higher side.05c17a517gfs-controller-service-token,workspace-files-controller-service-tokenandRPC_PROXY_MCP_HOST_EDGE_TOKEN. No call uses them with V2 off; they move to PR3. The token script,networkpolicies.yamland the two HCC templates are byte-identical todev.eb3d2401f76ba77d13,c01a0bdefa7d282ace561effa95dev4fd7b2878(#1425: desktop-app only, 33 files, no migration); no conflicts.378d2cdcfe611ebfcbmerge had left both atdev's numbers.0d1c00106dev05419da54(#1434). The only conflict was the mcp-host version line, resolved to 0.1.636.ce36151d4inTransaction = falsestores before returns (CodeQL alerts 2014–2018); a new test pins the statement that ends the transaction on each early return of the locked boundary step, which no test caught before; the wire test'smergePatchhelper rejects__proto__andconstructorkeys (alert 2007).73fcf94c8tsxchild process) inside test bodies and timed out under CPU load. Each file now runs it once per request in abeforeAll, with a 240 s limit per child, and rethrows a recorded failure only in the tests that use it. No test title, body or assertion changes.bd86ac5c7workflowRunStoreErrors.test.ts: theafterAllhook timed out in CI because a socket the pool opened late to the silent loopback server was never destroyed; the sockets are now destroyed after every listener stops. The closing-server capture now reads the startup packet before closing, so macOS no longer reportsECONNRESETinstead of an orderly close. Captured errors and verdicts unchanged.14100a24fdev61f32ae04(#1448: CI check for tracked files that a.gitignoreexcludes). No conflict; no service code.edb622ceadev0ca497d37(#1445, #1465, #1467). Version lines resolved to the higher side; the release manifest is synced to v0.12.1; duplicate path filters the merge left inbuild-publish.ymlare removed.3f28a505fpackages/action-context-contractsinto both stages. Their relative import of that package did not resolve inside an image built from the service directory (TS2307), sodev's Build & Publish would have failed after the merge. The import change in this commit is reverted by36071cc1a.cd6a00267dev, by recipe status, whatever its run row phase; a report for a run with no row is still refused. A bound run keeps the phase check.ab3d45491RPC_PROXY_MCP_SERVER_NAMESPACEdefaults tomcp-server, as control-api's own setting does, instead of stopping the boot in production when a manifest older than the image does not set it. Only the V2 path reads it, and its destination check fails closed on a wrong value.27c05b0144cf9a833c); this commit corrects the log line and the comment and adds a test that pins the skip.5a2a1333cdev's notes on migrations0115,0116and0121restored incontrol-api/src/db.ts;MCP_HOST_ACTIVITY_MAX_EVENT_BYTESmust be set to the same value on rpc-proxy and the Host; gfs-controller images of this release need migration0140before they run.36071cc1apackages/action-context-contractsby relative path again, as this branch did before3f28a505f, and drop thefile:dependency3f28a505fadded. The root-context image build stays.603103d92build-argskey in Build & Publish (see "Status").72f49fb99dev22dd4213c(#1460). The only conflict was the workflow-recipes version line, resolved to the branch side; the version hook then set 0.1.500.9622b5c7ddev34f14dd37(#1135, status-report bounding).devnow reads each WRC error answer once as text; this branch's 423 pause and 503 run-store checks read the body again and would have stopped detecting both codes, so they now parse the same text. Two tests pin the 423 detection through the reporter, which had none.6fa26f189clerum.io/workflow-run-authority: bound. Every V1runningreport is decided by recipe status again, without reading Postgres. The upgrade note says workflow-recipes needs migrations0139and0143.7d45f0012c9df7490bdev's again.b4566e383Retry-After; a Slack or Teams decision is sent again once, in the background, after the Host's wait.8da3e468c87b242c5f1387025e5Retry-Afteron the synchronous enrollment answer.b22ceb213054a382a1aa8e6a877dev193f5f4a7(#1339, HCC network-policy pass). Only version lines conflicted; host-context-controller is set to 0.1.446 (abovedev's 0.1.445) because this branch changes HCC code.No longer in PR2, for parity with
dev(already removed atc64989e7c): the 10/minute limiters on the Sandbox OAuth routesand the legacy Desktop/Sandbox session-creation admission. Earlier descriptions of this PR still listed them.
Migrations
PR2 registers
0139–0144, afterdev's last migration0138:0139_workflow_authority_bindings0140_gfs_upload_authority_bindings0141_pr2_readiness_evidence0142_pr2_runtime_privileges0143_workflow_run_failure_reason0144_workflow_authorization_pauseOpen PR #1205 also registers
0139–0141. Whichever lands second renumbers; this is noted on #1205.Behaviour with V2 off
V1 does not behave exactly as on
devwith the V2 flags off. These differences stay in this PR:(
mcp-host/src/server/directServiceAdmission.ts, not indev), then answers 429 withRetry-After. Charged: new messages,provider message authorization, tool approvals, workflow approval decide, resolve and result request, approval-medium enrollment
and Telegram verification. Not charged: the cron-result poll and its acknowledgement, the notification claim and its ack/fail, the
task-result read and the progress stream of an accepted task. Real peaks of the charged routes are measured before the merge.
action_context_disabledwhenactionContextV2is off, before anydelegation check (
requireActionContextEnabled). The Host message admission (Spec 62) and Host-RPC admission (Spec 65) charges aremade only in that checkpoint route, so V1 traffic is not charged by them.
runningreport reads the run store only for a run with an authority binding, which its child recipe carriesas the label
clerum.io/workflow-run-authority: bound. No such run exists with the flags off, so everyrunningreport is decidedby recipe status, as on
dev.DELETE /internal/sandbox-ui/oauth/grantneeds the recipe to exist and to declare the client once (404 / 400 and thegrant stays);
devdeleted the grant and answered 204. Follow-up: control-api: sandbox OAuth disconnect leaves the grant when the recipe or client declaration is gone #1476.and answers
recipe_not_found,unknown_oauth_clientorno_grantwhen the owner is gone, the client id is missing or duplicated,or the provider differs. A cached token that is not stale is still returned without reading the owner, as on
dev.host_wake_requestedalso for a Host that is already active, with different fields.cookie,authorization,x-evenfire-action-delegationandx-clerum-edge-*before proxying the legacyDesktop view (HTTP and WebSocket) to the desktop pod;
devforwarded every header. Follow-up: rpc-proxy: verify the legacy Desktop view after edge-credential stripping #1477.devkept writing.rpc-proxy/src/routes/rpc.tsandPOST /sandbox-ui/:recipeNs/:recipeName/oauth/authorize-url), so a token without the scope can get 400 or 413 instead of 403.Follow-up: rpc-proxy: seven legacy routes parse the body before the scope check #1478.
hostRef(400Invalid hostRef);devchecked it on the two artifactroutes only.
POST /v1/runtime/messagesruns after validation and admission.the cached session and initialises a new one.
path_invalid.Follow-up: gfs-controller: malformed percent-encoding in a copy id is not reported as
path_invalid#1480.gfs_upload_sessions.action_authorityon every upload-session create, so control-apimigration
0140must run first (the GKE pipeline orders it that way).and the user gets no reply.
devre-submitted it synchronously; the Host suppresses a second delivery with the same message id whilethe first task's record exists, so what the
devuser received in that case was not established. On every channel, as ondev, astatus call that returns no message id ends without a reply. Follow-up: channel-reader: an accepted async task gets no reply when the status message cannot be sent #1479.
Follow-up: channel-reader: Slack, Teams and email events refused with 429 are not re-driven #1481.
Retry-Afteron thesynchronous paths. A Slack or Teams decision is sent again once, in the background, after the Host's
Retry-After(1 to 60 s); thepending retry is kept in memory (at most 64) and is lost if the reader restarts. While it is pending, any later click by the same
user is not sent, so that user's first decision is the one sent; a click by another user is sent at once and, as on
dev, isaccepted only from the approval's requester.
Known and accepted: if
actionContextV2is turned off while V2 runs exist, workflow-recipes treats that 409 as a retryableinvalid_responseand pauses the run durably (AuthorizationPaused, reasonworkflow_authority_unavailable). The run does notretry by itself; it continues only after an explicit resume (no connection is held); PR3 follow-up. The trigger and decision paths do not
cancel the action budget on client abort; the action deadline still bounds the work. With V2 on, a bound run latched
Failedby atransient error cannot recover (409 at its next
runningreport); to decide before V2 is enabled.Verification (local)
On the commits after
14100a24f:dev0ca497d37at3f28a505f, for regressions and blast radius, in two independent lanes(control plane and data; runtime edge and deploy): 0 High, 3 Medium, 15 Low. Two of the three Medium are fixed in
cd6a00267andab3d45491. The third (no reply on Telegram when the status message cannot be sent) is not fixed:27c05b014changes nobehaviour, and the delivery moved to channel-reader: an accepted async task gets no reply when the status message cannot be sent #1479. The Low items are fixed in
5a2a1333c, listed under "Behaviour with V2 off", or movedto the follow-up issues below.
72f49fb99againstdev22dd4213c, five lanes (control-api authorization, data and workflows, runtime edge,security and deploy, contract and tests), with the external review of
14100a24ftriaged in the same pass: no Critical or High, andno fail-open path with the V2 flags off. Four Medium changed V1 behaviour (V1
runningreports reading Postgres; polling starvinguser actions in the Host limit; a lost Telegram reply when the progress stream is refused; the approval reader ignoring the Host's
429) and are fixed in
6fa26f189..054a382a1, after a written plan validated in two rounds. The V2-only Medium items are kept asgates for enabling V2.
6fa26f189..054a382a1: each package's build and full suite (workflow-recipes 3447, mcp-host 9072, channel-reader 423,workflow-approval-request-reader 122 tests); every planned mutation fails a test; the tests
72f49fb99had for the changed filespass on the new code except the ones each fix replaces; the old and new code were run on the same inputs, and every changed answer
belongs to one of the four fixes.
cd6a00267,ab3d45491,27c05b014: the tests3f28a505fhad for the changed files pass on the new code, except theassertion on rpc-proxy's source text that
ab3d45491replaces; each new test fails when its fix is removed.dev0ca497d37, upgraded in place to this branch(
3f28a505fplus one comment). Migrations0139–0144applied once on the existing volume; existing rows and workflow runsreadable; the WorkflowRecipe CRD accepts
authorization_paused; the HCC template change reached HCC and the Host; rpc-proxy RBACand Deployment changes live; health ok. Not established: that rpc-proxy was Ready before HCC and the Hosts (only the creation
order was).
scripts/minikube/t0.sh) on5a2a1333cand on72f49fb99:T0_RUN_EXIT=0,T0_AFFECTED_CHECKS=PASSon both.36071cc1a: the CI failure reproduced locally with the3f28a505fimports (same error, exit 1) and passes with the relativeimport, with no
node_modulesin gfs-controller or workspace-files-controller; both services' tests and builds pass (689 and130 tests); both images build, and the module that imports the package loads inside each image.
603103d92:actionlintpasses onbuild-publish.yml; the action drops the empty build-argument line of non-profile-ui images.--all-servicesagainstdevand against the previous head: valid.The battery below ran on
a7d282ace.scripts/minikube/t0.sh, basee611ebfcb)T0_EXIT=0,T0_AFFECTED_CHECKS=PASS, 13 affected packagesrun:step of the CI jobsshell-syntaxanddeploy-manifest-checkpassword-recovery-suite-contract.mjs verifytest-control-api-runtime-access-contract.sh)devand againstc64989e7cT2_PUBLIC_BASE_REF=e611ebfcb)PUBLIC_BOUNDARY_PASSverify:electrondevor inc64989e7cand absent heredevside: 38 lines, each traced to a named commit or a format difference;c64989e7cside: attributed per file to the commits of this sync (87 files) or to the planned catalog deletion (11 files), not line by lineThe added and changed shell scripts were run under
/bin/bash3.2 and bash 5.CodeQL, the 16 open alerts on
c64989e7c: 2 fixed ina7d282ace(alerts 1835 and 1937), 5 dismissed as won't fix for parity withdev(alerts 1926–1930), 8 as false positive (alerts 1803, 1804, 1931–1936) and 1 as used in tests (alert 797). Eight older dismissals
(alerts 1430–1433, 1445, 1447, 1448, 1598) cited the removed limiters; their comments now state the
devparity.CodeQL, the 12 alerts raised on
0d1c00106(2007–2018), which replace older alerts on code this sync moved: 6 fixed ince36151d4(2007, 2014–2018); 2 dismissed as false positive (2008, rate limiters CodeQL does not credit; 2012, the PostgreSQL
host_wakebucket)and 4 as won't fix for parity with
dev(2009, 2010, 2011, 2013). Alert 1646's dismissal comment cited a removed limiter and now statesthe
devparity.Before merge
72f49fb99; the fixes since are covered by "Verification".dev, then one T2 on a recreated branch profile, on the head that merges.build time of the
workflow_runsindex.authorization_pausedout of band, before the merge on clerum-dev and before promotion on clerum.The upgrade rehearsal listed here before is done (see "Verification").
Follow-up issues (not in this PR)
rpc-proxy: seven legacy routes parse the body before the scope check #1478 (scope check before body parsing on seven rpc-proxy routes), channel-reader: an accepted async task gets no reply when the status message cannot be sent #1479 (channel-reader result delivery when the status message
fails), gfs-controller: malformed percent-encoding in a copy id is not reported as
path_invalid#1480 (path_invalidfor malformed ids in gfs-controller), channel-reader: Slack, Teams and email events refused with 429 are not re-driven #1481 (channel-reader 429 redelivery for Slack, Teams and email).hashtextkey space across unrelated subsystems #1395, Workflow run start creates the child WorkflowRecipe on Kubernetes inside a locked transaction #1397–Investigate the prod control-api core-pool exhaustion of 2026-10-13 #1405 (connection and lock holds across Kubernetes calls, pool timeouts, andthe 2026-10-13 core-pool exhaustion).
type-checked).
pre-gate-syncdoes not build external-rest-api.RPC_PROXY_MCP_SERVER_NAMESPACEmust equalCONTROL_API_MCP_SERVERS_NAMESPACE.Items from earlier descriptions
control-api/src/services/hostRpcAdmission.ts,rpc-proxy/src/hostRpcPreflight.ts); #795's Host-ref authority changes were not re-checked in this sync. No open PR targets this branch.Earlier review rounds (R24–R69) and their evidence remain in this PR's comments and in the edit history of this description.
🤖 Generated with Claude Code