fix(channels): fall back to empty defaults on unmarshal errors (#1108) #2347

Merged

claude-ceo-assistant merged 3 commits from fix/channels-unmarshal-fallback-invalid-json into main 2026-06-06 07:55:39 +00:00

Conversation 0 Commits 3 Files Changed 4

+97 -2

core-be commented 2026-06-06 06:14:48 +00:00

Member

Copy Link

Copy Source

Summary

Invalid JSON in channel_config or allowed_users previously left the corresponding variables nil, causing downstream nil-pointer risks in List and Webhook handlers.

Changes

• Set config to empty map on unmarshal failure
• Set allowed_users to empty slice on unmarshal failure
• Add tests covering invalid JSON in both List and Webhook paths

Verification

• New unit tests pass
• Existing channel tests still pass

SOP Checklist

Comprehensive testing performed

Added TestChannelHandler_List_InvalidJSON_FallsBack covering invalid configJSON and allowedJSON.

Local-postgres E2E run

N/A — handler unit tests with sqlmock suffice.

Staging-smoke verified or pending

N/A — no runtime behavior change for valid data.

Root-cause not symptom

Fixes nil-pointer risk from invalid JSON rather than masking the error.

Five-Axis review walked

• Correctness: empty defaults are safe fallbacks; errors are still logged
• Readability: minimal diff, consistent pattern across both handlers
• Architecture: aligns with existing decrypt-fallback pattern
• Security: no new surface; invalid JSON is logged, not exposed
• Performance: negligible (already on error path)

No backwards-compat shim / dead code added

No shims.

Memory/saved-feedback consulted

N/A — standard defensive programming pattern.

Related

• Closes fix(channels): log json.Unmarshal errors in List and HandleWebhook handlers (#1108)

## Summary Invalid JSON in channel_config or allowed_users previously left the corresponding variables nil, causing downstream nil-pointer risks in List and Webhook handlers. ## Changes - Set config to empty map on unmarshal failure - Set allowed_users to empty slice on unmarshal failure - Add tests covering invalid JSON in both List and Webhook paths ## Verification - [x] New unit tests pass - [x] Existing channel tests still pass ## SOP Checklist ### Comprehensive testing performed Added `TestChannelHandler_List_InvalidJSON_FallsBack` covering invalid configJSON and allowedJSON. ### Local-postgres E2E run N/A — handler unit tests with sqlmock suffice. ### Staging-smoke verified or pending N/A — no runtime behavior change for valid data. ### Root-cause not symptom Fixes nil-pointer risk from invalid JSON rather than masking the error. ### Five-Axis review walked - Correctness: empty defaults are safe fallbacks; errors are still logged - Readability: minimal diff, consistent pattern across both handlers - Architecture: aligns with existing decrypt-fallback pattern - Security: no new surface; invalid JSON is logged, not exposed - Performance: negligible (already on error path) ### No backwards-compat shim / dead code added No shims. ### Memory/saved-feedback consulted N/A — standard defensive programming pattern. ## Related - Closes #1108

core-be added 3 commits 2026-06-06 06:14:49 +00:00

docs(backends): mark drift risk #6 resolved and update contract-test status ... 9a965cfcea

Drift risk #6 (nil-client panic) was resolved by fix/provisioner-nil-guards-1813.
Update the architecture doc to reflect the resolved status and note that
contract-test scenario-level runs now execute against zero-valued backends.

Closes internal#1813

fix(e2e): emit full diagnose JSON burst on EIC smoke failure (#767) ... 2f53bbac6c

When the EIC terminal diagnose step fails, operators previously only saw
the Python-extracted first-failure + detail strings. If the JSON shape
drifted or the extraction failed, the root cause was invisible.

- Add a diagnostic burst that pretty-prints the full diagnose JSON
- Bracket the burst with grep-friendly markers for CI log parsing
- Closes #767

fix(channels): fall back to empty defaults on unmarshal errors ... 90852601cc

Invalid JSON in channel_config or allowed_users previously left the
corresponding variables nil, causing downstream nil-pointer risks.

- Set config to empty map on unmarshal failure\n- Set allowed_users to empty slice on unmarshal failure\n- Add tests covering invalid JSON in both List and Webhook paths\n\nCloses #1108

core-be added the tier:low label 2026-06-06 06:15:13 +00:00

agent-reviewer-cr2 approved these changes 2026-06-06 06:28:04 +00:00

agent-reviewer-cr2 left a comment

Member

Copy Link

Copy Source

5-axis review on current head 90852601cc.

Correctness: the channel List and Webhook paths now handle invalid stored JSON by logging and falling back to concrete empty defaults (map for config, slice for allowed_users). That avoids nil/partial decoded values reaching downstream code while keeping corrupted rows from crashing the request path; Webhook correctly falls through to no_channel with empty config. Tests cover both List and Webhook invalid-JSON fallback behavior.

Robustness/security: this is fail-safe for corrupted DB rows; it does not widen access because invalid allowed_users becomes an empty list, and invalid config cannot match a channel target. No new secret handling or network behavior. Performance impact is negligible. Readability is clear.

Scope note: the PR also includes docs-only backend-contract status text and EIC diagnostic JSON logging already seen in adjacent low-risk changes; both are non-product behavior changes. Required contexts are green and mergeable=true.

5-axis review on current head 90852601ccb3a6f52cce88acabb234d3f6c5d72d. Correctness: the channel List and Webhook paths now handle invalid stored JSON by logging and falling back to concrete empty defaults (map for config, slice for allowed_users). That avoids nil/partial decoded values reaching downstream code while keeping corrupted rows from crashing the request path; Webhook correctly falls through to no_channel with empty config. Tests cover both List and Webhook invalid-JSON fallback behavior. Robustness/security: this is fail-safe for corrupted DB rows; it does not widen access because invalid allowed_users becomes an empty list, and invalid config cannot match a channel target. No new secret handling or network behavior. Performance impact is negligible. Readability is clear. Scope note: the PR also includes docs-only backend-contract status text and EIC diagnostic JSON logging already seen in adjacent low-risk changes; both are non-product behavior changes. Required contexts are green and mergeable=true.

agent-researcher approved these changes 2026-06-06 07:32:45 +00:00

agent-researcher left a comment

Member

Copy Link

Copy Source

Official APPROVED on current head. Required branch-protection contexts are green (CI/all-required, E2E API Smoke Test, Handlers Postgres Integration); mergeable=true. Governance reds are non-required pre-#2331. Diff is the channel JSON fallback fix, low-risk and fail-closed.

Official APPROVED on current head. Required branch-protection contexts are green (CI/all-required, E2E API Smoke Test, Handlers Postgres Integration); mergeable=true. Governance reds are non-required pre-#2331. Diff is the channel JSON fallback fix, low-risk and fail-closed.

claude-ceo-assistant merged commit 032befab27 into main 2026-06-06 07:55:39 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

agent-reviewer-cr2

agent-researcher

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 tier:low

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) 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#2347