Bank commands collapse request creation failures, transport errors, non-200 HTTP responses, invalid acknowledgments, and request-ID mismatches into the same “Bank unavailable” response without useful console diagnostics. This affects both T5 and T6 and prevents operators from identifying the failing layer.
In Ascension match #1346, Josh’s .b command was recorded and bank initialization completed. Telemetry requests successfully reached EC, but the console provided no explanation for the bank failure.
Both bank scripts receive HTTP diagnostics in bank_http() but discard them when returning undefined. The HTTP plugin already supplies status codes, curl codes, and timing diagnostics; acknowledgment parsing currently exposes validity without a useful failure reason.
Fix
Log startup information: bank script/plugin versions, effective endpoint, server ID, credential validation result, and pending journal count. Never include credential values.
Give each request a correlation ID, including balance requests, and log its action and outcome. Diagnostic correlation must preserve the existing API request-ID contract.
Distinguish request creation failure, transport failure, HTTP rejection, invalid JSON/schema, request-ID mismatch, and valid application rejection.
Include available HTTP status, curl code, and DNS/connect/TLS/total timings. Missing diagnostics must not cause another script error.
Extend acknowledgment parsing to return a bounded diagnostic reason when validation fails.
Log journal load/write/removal failures and settlement progress: decision, attempt count, pending duration, next retry delay, and eventual success.
Rate-limit repeated identical failures while logging the first failure, changes in failure reason, and recovery.
Exclude tokens, Authorization headers, raw bodies, and private configuration contents from all diagnostics. Only log validated, bounded response codes.
Preserve existing point accounting, durable decisions, retry behavior, and player-facing messages.
Validate on both games against successful requests, authentication rejection, timeout, malformed acknowledgment, ID mismatch, storage failure, and settlement recovery. Bump affected release versions.
Deployment note: Ascension’s installed inventory reported bank script 1.0.0 with the old checksum while the plugin was 2.0.0. Verify the updated GSC artifact reaches the test server; this match does not conclusively demonstrate failure of the endpoint change tracked in #13.
Bank commands collapse request creation failures, transport errors, non-200 HTTP responses, invalid acknowledgments, and request-ID mismatches into the same “Bank unavailable” response without useful console diagnostics. This affects both T5 and T6 and prevents operators from identifying the failing layer.
In [Ascension match #1346](https://ec.thewrightserver.net/matches/1346), Josh’s `.b` command was recorded and bank initialization completed. Telemetry requests successfully reached EC, but the console provided no explanation for the bank failure.
Both bank scripts receive HTTP diagnostics in `bank_http()` but discard them when returning `undefined`. The HTTP plugin already supplies status codes, curl codes, and timing diagnostics; acknowledgment parsing currently exposes validity without a useful failure reason.
**Fix**
- [x] Log startup information: bank script/plugin versions, effective endpoint, server ID, credential validation result, and pending journal count. Never include credential values.
- [x] Give each request a correlation ID, including balance requests, and log its action and outcome. Diagnostic correlation must preserve the existing API request-ID contract.
- [x] Distinguish request creation failure, transport failure, HTTP rejection, invalid JSON/schema, request-ID mismatch, and valid application rejection.
- [x] Include available HTTP status, curl code, and DNS/connect/TLS/total timings. Missing diagnostics must not cause another script error.
- [x] Extend acknowledgment parsing to return a bounded diagnostic reason when validation fails.
- [x] Log journal load/write/removal failures and settlement progress: decision, attempt count, pending duration, next retry delay, and eventual success.
- [x] Rate-limit repeated identical failures while logging the first failure, changes in failure reason, and recovery.
- [x] Exclude tokens, Authorization headers, raw bodies, and private configuration contents from all diagnostics. Only log validated, bounded response codes.
- [x] Preserve existing point accounting, durable decisions, retry behavior, and player-facing messages.
- [ ] Validate on both games against successful requests, authentication rejection, timeout, malformed acknowledgment, ID mismatch, storage failure, and settlement recovery. Bump affected release versions.
**Deployment note:** Ascension’s installed inventory reported bank script 1.0.0 with the old checksum while the plugin was 2.0.0. Verify the updated GSC artifact reaches the test server; this match does not conclusively demonstrate failure of the endpoint change tracked in #13.
Reviewed 3bb0b5178e. Not implemented; leaving open.
This review checks the current implementation against every acceptance criterion, regardless of commit references.
Checklist
Startup diagnostics. Current initialization has no combined versions/endpoint/server/credential-validation/journal-count summary.
Correlation for every action. Balance requests still use an empty API request ID and have no separate diagnostic correlation ID.
Failure classification. bank_http returns undefined for missing handles, transport/status failures and invalid/mismatched acknowledgments without identifying the layer.
HTTP/curl/timing details. bank_http receives diagnostics but only checks http_status; it discards the available timings and failure details.
Parser reason. Native bankResponse exposes valid and parsed fields, without a bounded validation-failure reason.
Storage/settlement detail. The native storage wrapper emits a generic storage error; bank_sender emits generic pending/retrying text without operation, attempt count or pending duration.
Rate limiting and recovery. The retry loop prints the same failure message repeatedly; there is no reason-change/recovery-aware log policy.
Safe diagnostic fields. The proposed diagnostics do not exist yet; their token/header/body exclusion and bounded-code handling remain to implement and validate.
Preserve behavior with instrumentation. Current accounting/retry behavior exists, but no diagnostic change is available to verify that it preserves these contracts.
Diagnostic validation and releases. Existing bank fixtures cover accounting failures, not the required classified/sanitized log output. No diagnostic release/version entry is present.
Review notes
Checked both bank scripts plus native bank registration/parsing/storage code. The portable bank suite and native tests passed earlier in this session at the same scripts revision; those results do not prove the missing diagnostics.
The public endpoint change in #13 does not add any of this observability. Verify the deployed GSC checksum before attributing old-artifact failures to that change.
Next steps
Instrument both games and the native parser/storage layer, add diagnostic assertions for each failure/recovery path, bump the affected components and verify the artifacts deployed to the test servers.
Reviewed 3bb0b5178e40ea6d322093283edb39f160542a07. Not implemented; leaving open.
This review checks the current implementation against every acceptance criterion, regardless of commit references.
**Checklist**
- [ ] **Startup diagnostics.** Current initialization has no combined versions/endpoint/server/credential-validation/journal-count summary.
- [ ] **Correlation for every action.** Balance requests still use an empty API request ID and have no separate diagnostic correlation ID.
- [ ] **Failure classification.** bank_http returns undefined for missing handles, transport/status failures and invalid/mismatched acknowledgments without identifying the layer.
- [ ] **HTTP/curl/timing details.** bank_http receives diagnostics but only checks http_status; it discards the available timings and failure details.
- [ ] **Parser reason.** Native bankResponse exposes valid and parsed fields, without a bounded validation-failure reason.
- [ ] **Storage/settlement detail.** The native storage wrapper emits a generic storage error; bank_sender emits generic pending/retrying text without operation, attempt count or pending duration.
- [ ] **Rate limiting and recovery.** The retry loop prints the same failure message repeatedly; there is no reason-change/recovery-aware log policy.
- [ ] **Safe diagnostic fields.** The proposed diagnostics do not exist yet; their token/header/body exclusion and bounded-code handling remain to implement and validate.
- [ ] **Preserve behavior with instrumentation.** Current accounting/retry behavior exists, but no diagnostic change is available to verify that it preserves these contracts.
- [ ] **Diagnostic validation and releases.** Existing bank fixtures cover accounting failures, not the required classified/sanitized log output. No diagnostic release/version entry is present.
**Review notes**
- Checked both bank scripts plus native bank registration/parsing/storage code. The portable bank suite and native tests passed earlier in this session at the same scripts revision; those results do not prove the missing diagnostics.
- The public endpoint change in #13 does not add any of this observability. Verify the deployed GSC checksum before attributing old-artifact failures to that change.
**Next steps**
- Instrument both games and the native parser/storage layer, add diagnostic assertions for each failure/recovery path, bump the affected components and verify the artifacts deployed to the test servers.
Reviewed 463b4bcc51. The diagnostic implementation is complete and portable checks pass; leaving open for the final live validation item.
Checklist
Startup diagnostics. Both bank scripts log script/plugin versions, fixed endpoint, bounded server identity, credential-format status, and loaded journal count. Failed loads report an unknown count and disable the bank.
Request correlation. Every HTTP attempt gets a separate diagnostic ID, including balance reads. Balance keeps the empty API acknowledgment ID; durable transfer IDs remain unchanged.
Failure classification.bank_http distinguishes creation, transport, HTTP rejection, absent diagnostics/response, parser failure, ID mismatch, unexpected state, and application rejection.
Transport details. HTTP/curl codes and DNS/connect/TLS/total timings are logged through guarded numeric accessors; missing or malformed values become unavailable.
Bounded parser reasons. Both native parsers expose fixed reasons for invalid JSON, schema/field limits, state/amount validation, and inconsistent acknowledgments without returning response text as a reason.
Journal and settlement progress. Load/write/remove results and decision, attempts, elapsed pending time, retry delay, and final resolution are covered in script and native logs.
Rate limiting. Script failures use fixed per-action keys and 30-second suppression; reason/status/code changes and recovery are immediate. Native storage failure/recovery logs are separately bounded.
Secret exclusion. Raw bodies, authorization headers, credentials, and exception text are excluded. Response codes/reasons use fixed allowlists; hostile-field fixtures and native adapter checks pass.
Accounting preserved. The original durable decisions, payload identities, retry delays, point changes, and player messages remain intact. Accounting, failed-write, replay, and actual EC-service integration checks pass.
Release and live acceptance. Version/changelog requirements are met: both bank scripts 2.0.2 and plugins 1.2.0. Portable scenarios cover the requested failures, but the complete in-game T5/T6 rejection/timeout/malformed-response/ID-mismatch/storage/recovery matrix has not been demonstrated.
Review notes
node tools/check_bank.mjs --ec-root /workspace/electric-cherry: 44 portable bank fixture groups passed, including the real EC bank service/migrations with isolated SQLite at EC revision 82e07ea33eedddd7b707f5e62c12d2e606a1e680.
python3 tools/test_plugin_bank.py --json-header ...: both T5 and T6 native storage/parser, separate-process replay, and registration-adapter checks passed with ASan/UBSan and each plugin's pinned JSON header. The default command initially lacked restored headers; the successful run used headers fetched at the exact recorded dependency revisions.
These are source/portable integration checks, not Windows DLL builds or live Plutonium failure injection. No production settings, journals, or server processes were changed.
Earlier monitoring in this conversation confirmed 2.0.2/1.2.0 startup and a successful .b on T5 Shang, plus successful .b, 500-point deposit and withdrawal on T6 Die Rise. That is useful success-path evidence, not completion of the failure matrix. Subsequent live 2.0.3 artifacts are not the repository revision reviewed here.
Next steps
On controlled T5/T6 test instances, verify the documented authentication, timeout, malformed acknowledgment, wrong ID, journal failure, and settlement-recovery scenarios. Record deployed hashes/versions, sanitized logs, rate limiting/recovery, and unchanged accounting/player messages. Do not disrupt public matches to perform these tests.
Reviewed 463b4bcc51871ef4f24f6360c35ff11c443be138. The diagnostic implementation is complete and portable checks pass; leaving open for the final live validation item.
**Checklist**
- [x] **Startup diagnostics.** Both bank scripts log script/plugin versions, fixed endpoint, bounded server identity, credential-format status, and loaded journal count. Failed loads report an unknown count and disable the bank.
- [x] **Request correlation.** Every HTTP attempt gets a separate diagnostic ID, including balance reads. Balance keeps the empty API acknowledgment ID; durable transfer IDs remain unchanged.
- [x] **Failure classification.** `bank_http` distinguishes creation, transport, HTTP rejection, absent diagnostics/response, parser failure, ID mismatch, unexpected state, and application rejection.
- [x] **Transport details.** HTTP/curl codes and DNS/connect/TLS/total timings are logged through guarded numeric accessors; missing or malformed values become unavailable.
- [x] **Bounded parser reasons.** Both native parsers expose fixed reasons for invalid JSON, schema/field limits, state/amount validation, and inconsistent acknowledgments without returning response text as a reason.
- [x] **Journal and settlement progress.** Load/write/remove results and decision, attempts, elapsed pending time, retry delay, and final resolution are covered in script and native logs.
- [x] **Rate limiting.** Script failures use fixed per-action keys and 30-second suppression; reason/status/code changes and recovery are immediate. Native storage failure/recovery logs are separately bounded.
- [x] **Secret exclusion.** Raw bodies, authorization headers, credentials, and exception text are excluded. Response codes/reasons use fixed allowlists; hostile-field fixtures and native adapter checks pass.
- [x] **Accounting preserved.** The original durable decisions, payload identities, retry delays, point changes, and player messages remain intact. Accounting, failed-write, replay, and actual EC-service integration checks pass.
- [ ] **Release and live acceptance.** Version/changelog requirements are met: both bank scripts 2.0.2 and plugins 1.2.0. Portable scenarios cover the requested failures, but the complete in-game T5/T6 rejection/timeout/malformed-response/ID-mismatch/storage/recovery matrix has not been demonstrated.
**Review notes**
- `node tools/check_bank.mjs --ec-root /workspace/electric-cherry`: 44 portable bank fixture groups passed, including the real EC bank service/migrations with isolated SQLite at EC revision 82e07ea33eedddd7b707f5e62c12d2e606a1e680.
- `python3 tools/test_plugin_bank.py --json-header ...`: both T5 and T6 native storage/parser, separate-process replay, and registration-adapter checks passed with ASan/UBSan and each plugin's pinned JSON header. The default command initially lacked restored headers; the successful run used headers fetched at the exact recorded dependency revisions.
- These are source/portable integration checks, not Windows DLL builds or live Plutonium failure injection. No production settings, journals, or server processes were changed.
- Earlier monitoring in this conversation confirmed 2.0.2/1.2.0 startup and a successful `.b` on T5 Shang, plus successful `.b`, 500-point deposit and withdrawal on T6 Die Rise. That is useful success-path evidence, not completion of the failure matrix. Subsequent live 2.0.3 artifacts are not the repository revision reviewed here.
**Next steps**
- On controlled T5/T6 test instances, verify the documented authentication, timeout, malformed acknowledgment, wrong ID, journal failure, and settlement-recovery scenarios. Record deployed hashes/versions, sanitized logs, rate limiting/recovery, and unchanged accounting/player messages. Do not disrupt public matches to perform these tests.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Bank commands collapse request creation failures, transport errors, non-200 HTTP responses, invalid acknowledgments, and request-ID mismatches into the same “Bank unavailable” response without useful console diagnostics. This affects both T5 and T6 and prevents operators from identifying the failing layer.
In Ascension match #1346, Josh’s
.bcommand was recorded and bank initialization completed. Telemetry requests successfully reached EC, but the console provided no explanation for the bank failure.Both bank scripts receive HTTP diagnostics in
bank_http()but discard them when returningundefined. The HTTP plugin already supplies status codes, curl codes, and timing diagnostics; acknowledgment parsing currently exposes validity without a useful failure reason.Fix
Deployment note: Ascension’s installed inventory reported bank script 1.0.0 with the old checksum while the plugin was 2.0.0. Verify the updated GSC artifact reaches the test server; this match does not conclusively demonstrate failure of the endpoint change tracked in #13.
Reviewed
3bb0b5178e. Not implemented; leaving open.This review checks the current implementation against every acceptance criterion, regardless of commit references.
Checklist
Review notes
Next steps
Reviewed
463b4bcc51. The diagnostic implementation is complete and portable checks pass; leaving open for the final live validation item.Checklist
bank_httpdistinguishes creation, transport, HTTP rejection, absent diagnostics/response, parser failure, ID mismatch, unexpected state, and application rejection.Review notes
node tools/check_bank.mjs --ec-root /workspace/electric-cherry: 44 portable bank fixture groups passed, including the real EC bank service/migrations with isolated SQLite at EC revision 82e07ea33eedddd7b707f5e62c12d2e606a1e680.python3 tools/test_plugin_bank.py --json-header ...: both T5 and T6 native storage/parser, separate-process replay, and registration-adapter checks passed with ASan/UBSan and each plugin's pinned JSON header. The default command initially lacked restored headers; the successful run used headers fetched at the exact recorded dependency revisions..bon T5 Shang, plus successful.b, 500-point deposit and withdrawal on T6 Die Rise. That is useful success-path evidence, not completion of the failure matrix. Subsequent live 2.0.3 artifacts are not the repository revision reviewed here.Next steps