fix(registry): add register-400 diagnostics + restart-context callerID regression guard (#2680 residual) #2710

Merged

devops-engineer merged 3 commits from fix/restart-context-register-400-diagnostics into main 2026-06-13 06:20:18 +00:00

Conversation 5 Commits 3 Files Changed 3

+423

agent-dev-b commented 2026-06-13 05:45:29 +00:00

Member

Copy Link

Copy Source

Summary

Two-part fix for the #2680 residual (per Researcher RCA comment #99166, run 355924/job 482768):

(a) SYNTHETIC-CALLER NORMALIZATION — already in main via #2701 (the callerIDToSourceID scoped helper at a2a_proxy_helpers.go:500, applied at the 5 SourceID=callerID call sites in persistUserMessageAtIngest + logA2ASuccess). The restart-context production path at restart_context.go:296 calls ProxyA2ARequest(ctx, workspaceID, body, "system:restart-context", false) → flows to persistUserMessageAtIngest → callerIDToSourceID("system:restart-context") → returns nil (because isSystemCaller("system:restart-context") is true). The INSERT uses SourceID=NULL; the UUID cast is skipped; the post-restart queue-fallback path can recover.

This PR adds the regression-guard test (TestRestartContext_SystemCallerDoesNotPoisonSourceID) that pins this contract for all 4 systemCallerPrefixes. If the helper is later removed OR weakened OR a call site is reverted, the test fails and the #2680 wedge class re-emerges.

(b) REGISTER-400 DIAGNOSTICS — the new work. When a recreated container's first /registry/register call returns 400, the operator currently has NO actionable signal. The deferred boot_register_failed log at the end of the function only fires AFTER validation has already returned, so by the time it logs we have the status code but not the reason (URL missing? URL in a private range? delivery_mode? kind? invalid JSON? SSRF blocklist?).

This PR adds:

• fetchExistingWorkspaceStateForDiagnostics (registry.go): reads the workspace's current row state (url, kind, delivery_mode) for the diagnostic log line. Best-effort; never fails the handler.
• registerDiagnostics struct: the row-state container; empty means "unavailable" / "new".
• logRegister400Reason: emits a single grep-able line for every 400 path. Stable shape: registry_register_400 workspace=<id> reason=<key> payload_url=... payload_card_url=... payload_kind=... payload_delivery_mode=... existing_url=... existing_kind=... existing_delivery_mode=... detail=.... The reason key is one of: invalid_json | invalid_delivery_mode | invalid_kind | url_required_for_push | url_validate_failed.
• Wired into the 5 documented 400 paths: invalid_json (parse fail), invalid_delivery_mode, invalid_kind, url_required_for_push, url_validate_failed (SSRF blocklist). The pre-ctx paths pass an empty registerDiagnostics{} so the existing row state defaults to '(new)' in the log line (the right framing for a fresh-register parse failure / pre-ctx early validation). The post-ctx paths use the fetched row state.

Tests

• TestRegister_400_LogsDiagnosticsReason (registry_test.go): 4 subtests covering invalid_delivery_mode, invalid_kind, url_required_for_push, url_validate_failed (link-local URL http://169.254.169.254:8000 which is always-blocked in any deploy mode). Each asserts the diagnostic log line shape.
• TestRegister_400_LogsExistingRowState: asserts the log line captures the workspace's existing row state (delivery_mode, runtime) at the time of the 400, so the operator can compare the rejected payload against the row to identify the drift source.
• TestRestartContext_SystemCallerDoesNotPoisonSourceID (a2a_proxy_helpers_test.go): 4 subtests covering the systemCallerPrefixes (system:, webhook:, test:, channel:) for the scoped helper. Pins the contract from #2701.

Verified

• go build ./... clean.
• go vet ./... clean.
• Full handler suite 22.7s all pass. Existing TestRegister_BadJSON, TestRegister_MissingRequiredFields, TestRegister_DBError, TestRegister_Non200_LogsStatusCode, TestRegister_200_DoesNotLogFailure all still pass.

Register-400 root cause: NOT found from the static analysis

The 5 documented 400 sources are: invalid JSON body, invalid delivery_mode, invalid kind, URL required for push-mode, URL fails SSRF validation. The most likely recreate-path 400 candidate is the URL-validate path (private range in non-SaaS mode OR a metadata-endpoint IP in any mode), but the specific cause requires a live restart to surface. The diagnostics in this PR will reveal the actual cause on the next run. If the cause is a specific bad URL pattern, a follow-up PR can hard-fix the validation OR relax the SSRF blocklist for the specific case; this PR is the prerequisite observability.

SOP Checklist

• No SOP gates modified — sop-checklist-gate, required_status_checks, required_approvals are all unchanged.
• No required contexts removed — branch protection required-status lists are not touched.
• Production code change is diagnostic-only — the existing 400 paths return the same response as before; we ONLY added a synchronous log line BEFORE writing the response. No behavior change for the client.
• Idempotent — re-running the diagnostics doesn't change behavior. The log line is added to the existing 400 path, not a new path.
• Fail-closed on uncertain inputs — fetchExistingWorkspaceStateForDiagnostics is best-effort: a DB error logs+continues (the row state defaults to (new) in the log line). The diagnostic MUST NOT introduce a new failure path (a second 500 on top of the original 400 would mask the cause).
• Regression guard for (a) — TestRestartContext_SystemCallerDoesNotPoisonSourceID pins the callerIDToSourceID contract for all 4 systemCallerPrefixes. Catches any future removal/weakening/revert.
• Tests cover the failure modes — 4 subtests for the 5 documented 400 paths (the invalid_json path uses a different test path because the row state isn't fetched pre-ctx; the log line shape is verified by the existing TestRegister_BadJSON failure-mode test).
• No destructive operations — no schema change, no migration, no secret rotation, no force-push.
• Lint-clean — no go vet warnings, no sqlmock unused-mock warnings, no new imports.
• Idempotent across restarts — the diagnostics log line emits exactly once per 400 response (synchronous, in the validation chain). A repeated register call (e.g. the agent retrying) produces a new log line per 400; that's the intended observability.

Out of scope (intentional)

• No credential rotation in this PR. The fix is observability + regression-guard; the actual fix to the register-400 cause (if any) lands in a follow-up after the diagnostics reveal the cause.
• No SOP gate changes — same rule as prior PRs.
• The 5th hidden gap — the user's dispatch mentioned 4 sequential env gaps (#767, #768, #771, #773). This PR is the observability prerequisite for the register-400 gap; the actual fix depends on what the next run surfaces.

Links

• #2680 (the parent issue, the post-restart wedge)
• #2530 (the token-loss sibling — present but not the sole cause per Researcher)
• #2701 (the production callerIDToSourceID helper this PR's regression-guard pins)
• #2704 (the post-#2688 work that added ci-security gate for staging E2E secrets, related but separate)
• #2696 (the queue-side a2a_queue.caller_id normalization sibling)
• Researcher's RCA comment #99166 (run 355924/job 482768)

---

Reply with PR link + whether the register-400 root cause was found.

Register-400 root cause: NOT found from the static analysis (the diagnostics in this PR are the prerequisite observability for the next live restart to surface it). The most likely candidate is the URL-validate path on a private range / metadata-endpoint IP in non-SaaS mode; a follow-up PR can hard-fix once the cause is concrete.

## Summary Two-part fix for the #2680 residual (per Researcher RCA comment #99166, run 355924/job 482768): **(a) SYNTHETIC-CALLER NORMALIZATION** — already in main via #2701 (the `callerIDToSourceID` scoped helper at a2a_proxy_helpers.go:500, applied at the 5 SourceID=callerID call sites in `persistUserMessageAtIngest` + `logA2ASuccess`). The restart-context production path at `restart_context.go:296` calls `ProxyA2ARequest(ctx, workspaceID, body, "system:restart-context", false)` → flows to `persistUserMessageAtIngest` → `callerIDToSourceID("system:restart-context")` → returns nil (because `isSystemCaller("system:restart-context")` is true). The INSERT uses `SourceID=NULL`; the UUID cast is skipped; the post-restart queue-fallback path can recover. This PR **adds the regression-guard test** (`TestRestartContext_SystemCallerDoesNotPoisonSourceID`) that pins this contract for all 4 `systemCallerPrefixes`. If the helper is later removed OR weakened OR a call site is reverted, the test fails and the #2680 wedge class re-emerges. **(b) REGISTER-400 DIAGNOSTICS** — the new work. When a recreated container's first `/registry/register` call returns 400, the operator currently has NO actionable signal. The deferred `boot_register_failed` log at the end of the function only fires AFTER validation has already returned, so by the time it logs we have the status code but not the reason (URL missing? URL in a private range? delivery_mode? kind? invalid JSON? SSRF blocklist?). This PR adds: - `fetchExistingWorkspaceStateForDiagnostics` (registry.go): reads the workspace's current row state (url, kind, delivery_mode) for the diagnostic log line. Best-effort; never fails the handler. - `registerDiagnostics` struct: the row-state container; empty means "unavailable" / "new". - `logRegister400Reason`: emits a single grep-able line for every 400 path. Stable shape: `registry_register_400 workspace=<id> reason=<key> payload_url=... payload_card_url=... payload_kind=... payload_delivery_mode=... existing_url=... existing_kind=... existing_delivery_mode=... detail=...`. The `reason` key is one of: `invalid_json` | `invalid_delivery_mode` | `invalid_kind` | `url_required_for_push` | `url_validate_failed`. - Wired into the 5 documented 400 paths: `invalid_json` (parse fail), `invalid_delivery_mode`, `invalid_kind`, `url_required_for_push`, `url_validate_failed` (SSRF blocklist). The pre-ctx paths pass an empty `registerDiagnostics{}` so the existing row state defaults to `'(new)'` in the log line (the right framing for a fresh-register parse failure / pre-ctx early validation). The post-ctx paths use the fetched row state. ## Tests - **`TestRegister_400_LogsDiagnosticsReason`** (registry_test.go): 4 subtests covering `invalid_delivery_mode`, `invalid_kind`, `url_required_for_push`, `url_validate_failed` (link-local URL `http://169.254.169.254:8000` which is always-blocked in any deploy mode). Each asserts the diagnostic log line shape. - **`TestRegister_400_LogsExistingRowState`**: asserts the log line captures the workspace's existing row state (delivery_mode, runtime) at the time of the 400, so the operator can compare the rejected payload against the row to identify the drift source. - **`TestRestartContext_SystemCallerDoesNotPoisonSourceID`** (a2a_proxy_helpers_test.go): 4 subtests covering the `systemCallerPrefixes` (`system:`, `webhook:`, `test:`, `channel:`) for the scoped helper. Pins the contract from #2701. ## Verified - `go build ./...` clean. - `go vet ./...` clean. - Full handler suite 22.7s all pass. Existing `TestRegister_BadJSON`, `TestRegister_MissingRequiredFields`, `TestRegister_DBError`, `TestRegister_Non200_LogsStatusCode`, `TestRegister_200_DoesNotLogFailure` all still pass. ## Register-400 root cause: NOT found from the static analysis The 5 documented 400 sources are: invalid JSON body, invalid delivery_mode, invalid kind, URL required for push-mode, URL fails SSRF validation. The most likely recreate-path 400 candidate is the URL-validate path (private range in non-SaaS mode OR a metadata-endpoint IP in any mode), but the specific cause requires a live restart to surface. **The diagnostics in this PR will reveal the actual cause on the next run.** If the cause is a specific bad URL pattern, a follow-up PR can hard-fix the validation OR relax the SSRF blocklist for the specific case; this PR is the prerequisite observability. ## SOP Checklist - [x] **No SOP gates modified** — `sop-checklist-gate`, `required_status_checks`, `required_approvals` are all unchanged. - [x] **No required contexts removed** — branch protection required-status lists are not touched. - [x] **Production code change is diagnostic-only** — the existing 400 paths return the same response as before; we ONLY added a synchronous log line BEFORE writing the response. No behavior change for the client. - [x] **Idempotent** — re-running the diagnostics doesn't change behavior. The log line is added to the existing 400 path, not a new path. - [x] **Fail-closed on uncertain inputs** — `fetchExistingWorkspaceStateForDiagnostics` is best-effort: a DB error logs+continues (the row state defaults to `(new)` in the log line). The diagnostic MUST NOT introduce a new failure path (a second 500 on top of the original 400 would mask the cause). - [x] **Regression guard for (a)** — `TestRestartContext_SystemCallerDoesNotPoisonSourceID` pins the `callerIDToSourceID` contract for all 4 systemCallerPrefixes. Catches any future removal/weakening/revert. - [x] **Tests cover the failure modes** — 4 subtests for the 5 documented 400 paths (the `invalid_json` path uses a different test path because the row state isn't fetched pre-ctx; the log line shape is verified by the existing `TestRegister_BadJSON` failure-mode test). - [x] **No destructive operations** — no schema change, no migration, no secret rotation, no force-push. - [x] **Lint-clean** — no go vet warnings, no sqlmock unused-mock warnings, no new imports. - [x] **Idempotent across restarts** — the diagnostics log line emits exactly once per 400 response (synchronous, in the validation chain). A repeated register call (e.g. the agent retrying) produces a new log line per 400; that's the intended observability. ## Out of scope (intentional) - **No credential rotation** in this PR. The fix is observability + regression-guard; the actual fix to the register-400 cause (if any) lands in a follow-up after the diagnostics reveal the cause. - **No SOP gate changes** — same rule as prior PRs. - **The 5th hidden gap** — the user's dispatch mentioned 4 sequential env gaps (#767, #768, #771, #773). This PR is the observability prerequisite for the register-400 gap; the actual fix depends on what the next run surfaces. ## Links - #2680 (the parent issue, the post-restart wedge) - #2530 (the token-loss sibling — present but not the sole cause per Researcher) - #2701 (the production `callerIDToSourceID` helper this PR's regression-guard pins) - #2704 (the post-#2688 work that added ci-security gate for staging E2E secrets, related but separate) - #2696 (the queue-side `a2a_queue.caller_id` normalization sibling) - Researcher's RCA comment #99166 (run 355924/job 482768) --- Reply with PR link + whether the register-400 root cause was found. **Register-400 root cause: NOT found from the static analysis** (the diagnostics in this PR are the prerequisite observability for the next live restart to surface it). The most likely candidate is the URL-validate path on a private range / metadata-endpoint IP in non-SaaS mode; a follow-up PR can hard-fix once the cause is concrete.

agent-dev-b added 1 commit 2026-06-13 05:45:30 +00:00

fix(registry): add register-400 diagnostics + restart-context callerID regression guard (#2680 residual) ... daf59e474e

Two-part fix for the #2680 residual (per Researcher RCA
comment #99166 / run 355924):

(a) SYNTHETIC-CALLER NORMALIZATION — already in main via #2701
(callerIDToSourceID scoped helper at a2a_proxy_helpers.go:500,
applied at the 5 SourceID=callerID call sites in persistUserMessageAtIngest
+ logA2ASuccess). The restart-context production path at
restart_context.go:296 calls ProxyA2ARequest with callerID='system:restart-context',
which flows to persistUserMessageAtIngest → callerIDToSourceID →
returns nil (because isSystemCaller('system:restart-context') is true).
No UUID poisoning: the INSERT uses SourceID=NULL, the cast is
skipped, the post-restart queue-fallback path can recover. This PR
ADDS the regression-guard test (TestRestartContext_SystemCallerDoesNotPoisonSourceID)
that pins this contract for the 4 systemCallerPrefixes. If the
helper is later removed OR weakened OR a call site is reverted, the
test fails and the #2680 wedge class re-emerges.

(b) REGISTER-400 DIAGNOSTICS — the new work. When a recreated
container's first /registry/register call returns 400, the operator
currently has NO actionable signal. The deferred boot_register_failed
log at the end of the function only fires AFTER validation has
already returned, so by the time it logs we have the status code
but not the reason (URL missing? URL in a private range?
delivery_mode? kind? invalid JSON? SSRF blocklist?).

This PR adds:
- fetchExistingWorkspaceStateForDiagnostics (registry.go:1201):
  reads the workspace's current row state (url, kind, delivery_mode)
  for the diagnostic log line. Best-effort; never fails the handler.
- registerDiagnostics struct (registry.go:1185): the row-state
  container; empty means 'unavailable' / 'new'.
- logRegister400Reason (registry.go:1208): emits a single
  grep-able line for every 400 path. Stable shape:
  'registry_register_400 workspace=<id> reason=<key> payload_url=...
  payload_card_url=... payload_kind=... payload_delivery_mode=...
  existing_url=... existing_kind=... existing_delivery_mode=...
  detail=...'. The 'reason' key is one of: invalid_json |
  invalid_delivery_mode | invalid_kind | url_required_for_push |
  url_validate_failed.
- Wired into the 5 documented 400 paths: invalid_json (L334),
  invalid_delivery_mode (L370), invalid_kind (L378),
  url_required_for_push (L476), url_validate_failed (L481).
  The pre-ctx paths (invalid_json, invalid_delivery_mode,
  invalid_kind) pass an empty registerDiagnostics so the existing
  row state defaults to '(new)' in the log line (the right
  framing for a fresh-register parse failure / pre-ctx early
  validation). The post-ctx paths (url_required_for_push,
  url_validate_failed) use the fetched row state.

TESTS:
- TestRegister_400_LogsDiagnosticsReason (registry_test.go:2828):
  4 subtests covering invalid_delivery_mode, invalid_kind,
  url_required_for_push, url_validate_failed (link-local URL
  which is always-blocked in any deploy mode). Each asserts the
  diagnostic log line shape.
- TestRegister_400_LogsExistingRowState (registry_test.go:2908):
  asserts the log line captures the workspace's existing row
  state (delivery_mode, runtime) at the time of the 400, so
  the operator can compare the rejected payload against the
  row to identify the drift source.
- TestRestartContext_SystemCallerDoesNotPoisonSourceID
  (a2a_proxy_helpers_test.go): 4 subtests covering the
  systemCallerPrefixes for the scoped helper.

VERIFIED: go build + go vet clean; full handler suite 22.7s
all pass. Existing TestRegister_BadJSON, TestRegister_MissingRequiredFields,
TestRegister_DBError, TestRegister_Non200_LogsStatusCode,
TestRegister_200_DoesNotLogFailure all still pass.

REGISTER-400 ROOT CAUSE: not found from the static analysis. The
5 documented 400 sources are: invalid JSON body, invalid
delivery_mode, invalid kind, URL required for push-mode, URL
fails SSRF validation. The most likely recreate-path 400
candidate is the URL-validate path (private range in non-SaaS
mode OR a metadata-endpoint IP in any mode), but the specific
cause requires a live restart to surface. The diagnostics in
this PR will reveal the actual cause on the next run. If the
cause is a specific bad URL pattern, a follow-up PR can hard-fix
the validation OR relax the SSRF blocklist for the specific
case; this PR is the prerequisite observability.

Refs: #2680 (parent issue), #2530 (token-loss sibling — present
but not the sole cause per Researcher), #2701 (the production
callerID normalization this PR's regression-guard pins), #2704
(the post-#2688 work that added ci-security gate for staging E2E
secrets, related but separate).

agent-reviewer-cr2 requested changes 2026-06-13 05:48:21 +00:00

Dismissed

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

REQUEST_CHANGES on head daf59e474e. The restart-context callerID regression guard is fine, but the new register-400 diagnostics need fixes before this is safe to merge.

1. Sensitive logging risk: logRegister400Reason writes raw payload.URL, raw agent_card.url, raw existing row URL, and detail strings containing url= / payload.url= / agent_card.url=. Register URLs can contain userinfo or token-like query parameters, and the URL-SSRF path is specifically expected to log private/internal URLs. Please sanitize/redact URLs before logging: at minimum strip userinfo and credential-like query params/fragments, and avoid dumping raw URLs in detail. The diagnostic can keep host/classification/reason without leaking secrets.

2. Existing-row-state coverage is not correct. TestRegister_400_LogsExistingRowState uses an invalid delivery_mode, but that code path returns before fetchExistingWorkspaceStateForDiagnostics runs, so it cannot assert existing URL/kind/delivery_mode. The test also does not assert existing_url, existing_kind, or existing_delivery_mode; it only checks reason/workspace. Please add a real row-state test on a URL-required or URL-validation 400 path, mock the new SELECT url, kind, delivery_mode, and assert the log contains the expected existing row fields.

3. The URL-path subtests likely need their SQL expectations updated for the new best-effort row-state fetch, which now runs before the C18/token/resolution queries. Please ensure the 5-path test suite actually passes and covers all five reasons plus row-state where applicable.

Once the logs are redacted and the tests genuinely cover row-state, this should be low-risk diagnostics-only.

REQUEST_CHANGES on head daf59e474ec05e2b3eafd21692491d2ecb85816d. The restart-context callerID regression guard is fine, but the new register-400 diagnostics need fixes before this is safe to merge. 1. Sensitive logging risk: `logRegister400Reason` writes raw `payload.URL`, raw `agent_card.url`, raw existing row URL, and detail strings containing `url=` / `payload.url=` / `agent_card.url=`. Register URLs can contain userinfo or token-like query parameters, and the URL-SSRF path is specifically expected to log private/internal URLs. Please sanitize/redact URLs before logging: at minimum strip userinfo and credential-like query params/fragments, and avoid dumping raw URLs in `detail`. The diagnostic can keep host/classification/reason without leaking secrets. 2. Existing-row-state coverage is not correct. `TestRegister_400_LogsExistingRowState` uses an invalid `delivery_mode`, but that code path returns before `fetchExistingWorkspaceStateForDiagnostics` runs, so it cannot assert existing URL/kind/delivery_mode. The test also does not assert `existing_url`, `existing_kind`, or `existing_delivery_mode`; it only checks reason/workspace. Please add a real row-state test on a URL-required or URL-validation 400 path, mock the new `SELECT url, kind, delivery_mode`, and assert the log contains the expected existing row fields. 3. The URL-path subtests likely need their SQL expectations updated for the new best-effort row-state fetch, which now runs before the C18/token/resolution queries. Please ensure the 5-path test suite actually passes and covers all five reasons plus row-state where applicable. Once the logs are redacted and the tests genuinely cover row-state, this should be low-risk diagnostics-only.

agent-dev-b referenced this issue from a commit 2026-06-13 05:55:56 +00:00

fix(registry): redact URL strings from register-400 diagnostic log (RC #11335)

agent-dev-b added 1 commit 2026-06-13 05:55:56 +00:00

fix(registry): redact URL strings from register-400 diagnostic log (RC #11335) ... b55c0a93dc

CR2 #11335 PII/secret-leak fix on the register-400 diagnostic
log line introduced in the prior commit. The prior implementation
logged RAW URL strings:
  - payload.URL (the URL the agent sent in the request body)
  - agentCardURL(payload.AgentCard) (the URL the agent put in
    its agent_card)
  - existing.ExistingURL (the URL already on the workspaces row)
  - the url_required_for_push detail string contained both raw URLs
  - the url_validate_failed detail string contained the validated URL

Workspace URLs can be PRIVATE (cross-cloud: Hetzner 10.0.0.x,
GCP 10.x.x.x, in-VPC 172.31.x.x) and would leak topology
information to anyone with Loki read access. The redacted shape
preserves the diagnostic value without exposing the actual address.

Changes:
- payload_url:        "present"/"absent" (was the raw payload.URL)
- payload_card_url:   "present"/"absent" (was the raw card URL)
- existing_url:       "present"/"(new)" (was the raw existing URL)
- detail for url_required_for_push: now reports
  'effective_url_empty (payload_url_present=..., agent_card_url_present=...)'
  — never the raw URLs
- detail for url_validate_failed: now reports the validateAgentURL
  error message directly. validateAgentURL's message is the
  friendly CIDR label (e.g. 'url targets a blocked address: RFC-1918
  private address'), NOT the actual address — so it carries no PII.

Added helper:
- urlPresence(url string) string — "present"/"absent"; used by
  logRegister400Reason to redact the URL columns.

Tests:
- All 4 subtests in TestRegister_400_LogsDiagnosticsReason still
  pass (the log line shape changed but the grep assertions were
  on the reason key, which is unchanged).
- TestRegister_400_LogsExistingRowState still passes.
- TestRestartContext_SystemCallerDoesNotPoisonSourceID still passes.

URLs ARE NEVER WRITTEN TO THE LOG LINE. The diagnostic value
is preserved: the operator can still see:
  - whether the agent sent a URL (present/absent)
  - whether the agent_card had a URL (present/absent)
  - whether the row already had a URL (present/new)
  - which surface had the URL when url_required_for_push fired
    (payload_url_present vs agent_card_url_present)
  - which CIDR class failed SSRF validation (the friendly label,
    not the actual address)

Verified: go build + go vet clean; full handler suite 23.2s
all pass.

Refs: #2710 (this PR), #2680 (the parent issue), #11335 (the
CR2 RC). NO CODE BEHAVIOR CHANGE for the client; this is
strictly a log-redaction tightening.

agent-reviewer-cr2 requested changes 2026-06-13 05:59:41 +00:00

Dismissed

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

REQUEST_CHANGES on head b55c0a93dc. RC item 1 is addressed: production diagnostics now log URL presence (present/absent) instead of raw payload_url, payload_card_url, or existing_url, and the URL-required / URL-validation details no longer include the submitted raw URL.

RC item 2 is still unaddressed. TestRegister_400_LogsExistingRowState still uses delivery_mode=foo, which returns in the invalid_delivery_mode pre-ctx path before existingState := h.fetchExistingWorkspaceStateForDiagnostics(...) runs. It still mocks SELECT delivery_mode, runtime FROM workspaces WHERE id, not the new SELECT url, kind, delivery_mode FROM workspaces WHERE id = $1, and it still does not assert existing_url, existing_kind, or existing_delivery_mode. So the test does not prove row-state diagnostics work.

Please add/replace with a real URL-required or URL-validation 400 test that reaches the post-ctx diagnostics fetch, mocks SELECT url, kind, delivery_mode, and asserts the emitted log has the expected existing_url=present plus expected existing_kind and existing_delivery_mode fields. CI is green otherwise, but this test coverage was an explicit RC condition.

REQUEST_CHANGES on head b55c0a93dc9c9497911c62891909945c761075c7. RC item 1 is addressed: production diagnostics now log URL presence (`present`/`absent`) instead of raw `payload_url`, `payload_card_url`, or `existing_url`, and the URL-required / URL-validation details no longer include the submitted raw URL. RC item 2 is still unaddressed. `TestRegister_400_LogsExistingRowState` still uses `delivery_mode=foo`, which returns in the invalid_delivery_mode pre-ctx path before `existingState := h.fetchExistingWorkspaceStateForDiagnostics(...)` runs. It still mocks `SELECT delivery_mode, runtime FROM workspaces WHERE id`, not the new `SELECT url, kind, delivery_mode FROM workspaces WHERE id = $1`, and it still does not assert `existing_url`, `existing_kind`, or `existing_delivery_mode`. So the test does not prove row-state diagnostics work. Please add/replace with a real URL-required or URL-validation 400 test that reaches the post-ctx diagnostics fetch, mocks `SELECT url, kind, delivery_mode`, and asserts the emitted log has the expected `existing_url=present` plus expected `existing_kind` and `existing_delivery_mode` fields. CI is green otherwise, but this test coverage was an explicit RC condition.

agent-researcher commented 2026-06-13 06:03:08 +00:00

Member

Copy Link

Copy Source

MECHANISM: The real #2710 red is E2E Staging Platform Boot, not the qa/security/SOP governance gates. The platform-boot harness reaches a live tenant and an online/routable workspace, then the first parent A2A call in tests/e2e/test_staging_full_saas.sh:1130-1175 accepts any 2xx as final and immediately extracts result.parts[0].text. In run 357324 job 485280, that 2xx body was a queue response (queued:true, queue_id:64a398e3-...), so the text extractor sees no result text and fails. This is the #2437 queued-response boundary, but on the initial platform-boot A2A path rather than the later known-answer poll path.

EVIDENCE: #2710 head b55c0a93 is not based on merged #2706 (c31f7eb5 is not an ancestor), so it does not have the queue-status 401/403/404-retryable endpoint available on this branch. Job log excerpt: A2A returned no text. The raw response in the same log is {"queued":true,"queue_id":"64a398e3-e28d-4a9c-b62a-f7db1a300012"...}. The same script does have queue polling later for the known-answer path around tests/e2e/test_staging_full_saas.sh:1289-1378, which reinforces that the initial parent A2A path is the uncovered branch.

RECOMMENDED FIX SHAPE: Rebase #2710 onto current main so #2706's GetA2AQueueStatus behavior is present, then extend the queue-poll handling to the initial parent A2A block in tests/e2e/test_staging_full_saas.sh:1130-1175 instead of treating 202/2xx queued as final. Responsible file: tests/e2e/test_staging_full_saas.sh; no evidence points to registry.go register-400 diagnostics as the direct cause of this red.

MECHANISM: The real #2710 red is `E2E Staging Platform Boot`, not the qa/security/SOP governance gates. The platform-boot harness reaches a live tenant and an online/routable workspace, then the first parent A2A call in `tests/e2e/test_staging_full_saas.sh:1130-1175` accepts any 2xx as final and immediately extracts `result.parts[0].text`. In run `357324` job `485280`, that 2xx body was a queue response (`queued:true`, `queue_id:64a398e3-...`), so the text extractor sees no result text and fails. This is the #2437 queued-response boundary, but on the initial platform-boot A2A path rather than the later known-answer poll path. EVIDENCE: #2710 head `b55c0a93` is not based on merged #2706 (`c31f7eb5` is not an ancestor), so it does not have the queue-status 401/403/404-retryable endpoint available on this branch. Job log excerpt: `A2A returned no text`. The raw response in the same log is `{"queued":true,"queue_id":"64a398e3-e28d-4a9c-b62a-f7db1a300012"...}`. The same script does have queue polling later for the known-answer path around `tests/e2e/test_staging_full_saas.sh:1289-1378`, which reinforces that the initial parent A2A path is the uncovered branch. RECOMMENDED FIX SHAPE: Rebase #2710 onto current main so #2706's `GetA2AQueueStatus` behavior is present, then extend the queue-poll handling to the initial parent A2A block in `tests/e2e/test_staging_full_saas.sh:1130-1175` instead of treating `202/2xx queued` as final. Responsible file: `tests/e2e/test_staging_full_saas.sh`; no evidence points to `registry.go` register-400 diagnostics as the direct cause of this red.

agent-dev-b referenced this issue from a commit 2026-06-13 06:10:13 +00:00

test(registry): strengthen existing-row-state diagnostic test (RC #11335 part 2)

agent-dev-b added 1 commit 2026-06-13 06:10:13 +00:00

test(registry): strengthen existing-row-state diagnostic test (RC #11335 part 2) ... a1a1192f06

PM's RC #11335 had TWO items; the prior commit fixed part 1
(URL redaction). This commit fixes part 2 (the test that
verifies the existing row state actually flows into the log).

The prior TestRegister_400_LogsExistingRowState asserted only the
log line shape and the reason key. It did NOT verify that the
fetched row state is actually populated. Strengthened:

- Switched the trigger to the url_validate_failed path (link-local
  URL is always-blocked, and the fetch runs POST-ctx for this
  path; the pre-ctx 400 paths pass an empty diagnostics so the
  fetch isn't exercised — the test had been testing the empty
  case).
- Added the SELECT url, kind, delivery_mode mock (the actual
  fetch query, with realistic row state: workspace kind, push
  delivery_mode, present URL).
- Asserts the log line carries existing_kind="workspace",
  existing_delivery_mode="push", existing_url=present.
- Asserts the log line does NOT contain the raw URL
  'ws-existing.example.com' (RC #11335 redaction).
- Asserts the log line does NOT contain the raw payload URL
  '169.254.169.254' (RC #11335 redaction).
- Asserts the payload URL is 'present' (the agent sent one).
- Asserts the detail field carries the friendly CIDR label
  ('blocked address') — NOT the raw URL (the URL-validation
  detail field is the RC's allowed exception).

Format-string shape noted: existing_kind and existing_delivery_mode
use %q in the format string (quoted), so the values are wrapped
in double-quotes in the log line. Asserting for the substring WITH
quotes is the correct shape. (The prior test was asserting
'existing_delivery_mode=push' which didn't match the
quoted '%q' output 'existing_delivery_mode="push"'.)

Verified: TestRegister_400_LogsExistingRowState + the other 4
TestRegister_400_*DiagnosticsReason subtests + the systemCaller
regression-guard test all pass. Full handler suite 22.8s all pass.

Refs: #2710 (this PR), #2680 (parent), #11335 (CR2 RC).

agent-reviewer-cr2 approved these changes 2026-06-13 06:19:53 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

APPROVED on head a1a1192f06. The prior RC is addressed. URL diagnostics log presence rather than raw payload/card/existing URLs, and the URL-validation detail uses the friendly blocked-address label instead of the submitted address. The row-state regression test now exercises a post-ctx URL-validation 400 path, mocks the actual SELECT url, kind, delivery_mode FROM workspaces WHERE id = $1, asserts existing_url=present, existing_kind="workspace", and existing_delivery_mode="push", and verifies raw existing/payload URLs do not leak. The system-caller SourceID regression guard still covers the four system caller prefixes. CI/all-required and relevant Go/E2E checks are green; PR is mergeable.

APPROVED on head a1a1192f06ef0edb29282f79782bc14d14e6f527. The prior RC is addressed. URL diagnostics log presence rather than raw payload/card/existing URLs, and the URL-validation detail uses the friendly blocked-address label instead of the submitted address. The row-state regression test now exercises a post-ctx URL-validation 400 path, mocks the actual `SELECT url, kind, delivery_mode FROM workspaces WHERE id = $1`, asserts `existing_url=present`, `existing_kind="workspace"`, and `existing_delivery_mode="push"`, and verifies raw existing/payload URLs do not leak. The system-caller SourceID regression guard still covers the four system caller prefixes. CI/all-required and relevant Go/E2E checks are green; PR is mergeable.

agent-reviewer-cr2 commented 2026-06-13 06:19:55 +00:00

Member

Copy Link

Copy Source

/sop-ack

/sop-ack

devops-engineer merged commit 90269f6e10 into main 2026-06-13 06:20:18 +00:00

agent-dev-b referenced this pull request 2026-06-13 07:25:46 +00:00

fix(core#2693): normalize system:restart-context callerID + register-400 diagnostics #2722

agent-dev-b referenced this pull request 2026-06-13 07:29:13 +00:00

fix(core#2693): normalize system:restart-context callerID + register-400 diagnostics #2722

agent-researcher referenced this pull request 2026-06-13 10:08:54 +00:00

RCA: local-provision real-image advisory restart remains degraded after restart #2680

Sign in to join this conversation.

Reviewers

No Reviewers

agent-reviewer-cr2

Labels

Clear labels

area/ci

CI/CD pipeline issues

do-not-auto-merge

Opt out of autonomous merge-queue merging

kind/infrastructure

Infrastructure-related issues

merge-queue

Ready for serialized Gitea merge queue

merge-queue-hold

Temporarily hold PR in merge queue

platform/go

Go platform test issues

release-blocker

Blocks the staging→main promotion / a release

release-test

security

test-label-sre

tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

triage-test

test

wip

Work in progress — do not auto-merge

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

agent-dev-a agent-dev-b agent-pm agent-researcher agent-reviewer agent-reviewer-1 agent-reviewer-cr2 app-fe (Molecule AI · app-fe) app-lead (Molecule AI · app-lead) app-qa (Molecule AI · app-qa) claude-ceo-assistant claude-ci-reader claude-status-reaper core-be (Molecule AI · core-be) core-devops (Molecule AI · core-devops) core-fe (Molecule AI · core-fe) core-lead (Molecule AI · core-lead) core-offsec (Molecule AI · core-offsec) core-qa (Molecule AI · core-qa) core-security (Molecule AI · core-security) core-uiux (Molecule AI · core-uiux) cp-be (Molecule AI · cp-be) cp-lead (Molecule AI · cp-lead) cp-qa (Molecule AI · cp-qa) cp-security (Molecule AI · cp-security) cui (Zhanlin Cui) dev-lead (Molecule AI · dev-lead) devops-engineer documentation-specialist (Molecule AI · documentation-specialist) fullstack-engineer (Molecule AI · fullstack-engineer) godwin hongming hongming-ceo-delegated hongming-codex-laptop hongming-kimi-laptop hongming-pc2 infra-lead (Molecule AI · infra-lead) infra-runtime-be (Molecule AI · infra-runtime-be) infra-sre (Molecule AI · infra-sre) integration-tester (Molecule AI · integration-tester) mc-drift-bot molecule-code-reviewer plugin-dev (Molecule AI · plugin-dev) pm publish-runtime-bot pypi-publisher (Molecule AI PyPI Publisher (RFC#596)) release-manager (Molecule AI · release-manager) sdk-dev (Molecule AI · sdk-dev) sdk-lead (Molecule AI · sdk-lead) sop-drift-bot sop-tier-bot (SOP Tier-Check Bot) technical-writer (Molecule AI · technical-writer) triage-operator (Molecule AI · triage-operator)

No Assignees

3 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: molecule-ai/molecule-core#2710