Integrate UCI lint, versioning and go check#1
Merged
Conversation
Add common UCI workflows to lint, stale check and version.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
bdchatham
added a commit
that referenced
this pull request
May 12, 2026
Cross-review by platform-engineer / kubernetes-specialist / security-specialist surfaced 5 must-fix items and several minor cleanups. All applied here. Drop mutex on Engine.SetExecutionConfig / ExecutionConfig (platform-engineer N1): the field is set-once during single-threaded startup before any goroutines run. The mutex was theater; dropping it matches the actual usage contract. Doc comment now says "not safe to call concurrently with task execution" explicitly. Wipe SEI_KEYRING_PASSPHRASE immediately after os.Getenv (security-specialist #2): previously the wipe happened after the passphrase-required check, which is benign today (value is "" at that point) but future-fragile. Moving the Unsetenv to immediately after Getenv closes the /proc/<pid>/environ window unconditionally on every return path. Sever wrapped error chain in OpenKeyring (platform-engineer N4 + security-specialist #3): use errors.New with redacted message instead of %w-wrapping the underlying SDK error. This prevents a typed field that might embed the passphrase from resurfacing via a future caller's %w or %v of a wrapped struct. Removed the %w wrap at buildExecutionConfig's call site too. Surface suffix-strip behavior in operator-facing docs (platform-engineer N3 + kubernetes-specialist n2): document that for the file backend, a trailing /keyring-file segment is stripped before handoff to the SDK. Also clarify the home-dir resolution (--home flag defaulting to $SEI_HOME) rather than implying a direct $SEI_HOME read. Tighten SmokeTestKeyring panic-recovery comment (platform-engineer N5): the previous comment claimed fail-fast was bypassed without recovery, but a panic IS fail-fast. The real reason for recovery is so the bounded retry loop can run. Updated. Add comment near kr.List() in smokeTestAttempt (security-specialist nit): explicit "discard the slice deliberately; List() decrypts the index, not individual keys" — documents the intentional non-destructiveness of the check. Deferred to follow-ups (out of #162 scope, documented in PR comment): - Rehydration-vs-config ordering trap (k8s N4) — fixes with #163 when sign-tx handlers exist - ExecutionConfig blast radius / per-handler capabilities (security #7) — design-trajectory work - Stdout-capturing log-line redaction test (platform #6, security #6) — seilog caches its writer at init time, making runtime redirect trivially pass-through; deferred to code-inspection at the single log site (serve.go:185) - shareProcessNamespace doc-vs-code consistency (security #1) — meta issue, not a vuln in this PR; noted in #221 thread Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
bdchatham
added a commit
that referenced
this pull request
May 12, 2026
3-specialist cross-review surfaced 6 must-fix items and 2 should-also-fix items. All applied here. 1. Cancellation produces false 'completed' (k8s B1): pollForInclusion now distinguishes ctx cancellation from deadline. Cancellation returns ctx.Err() so the engine records the task as failed rather than synthesizing a 'completed' record on a truncated run. Next Submit with the same TaskID short-circuits via the persisted checkpoint to the chain-query path. 2. Poll not ctx-aware on network call (platform B1): Check ctx + deadline BEFORE every QueryTx call, not just after. A CLI-driven --timeout 5s now actually bounds a hung-seid wait, independent of the QueryTx transport timeout. 3. Guard ordering (platform B2): Move Checkpoints nil-check BEFORE Keyring.Key() access. A misconfigured checkpoint store no longer triggers an unnecessary passphrase-prompt path on the file backend. 4. Zero-fee accepted (security #1): checkFeesDenom now rejects 0-resolved coins. '0usei' after ParseCoinsNormalized filters to empty Coins; we additionally check !IsAllPositive(). Catches bypasses of the min-gas-price contract. 5. Memo unbounded (security #2): New validateMemo: cap 256 bytes (matches Cosmos MaxMemoCharacters), reject non-printable. A 64KB memo could pollute the result store pre-broadcast; control-char injection could contaminate audit logs. 6. Checkpoint race on same TaskID + different ChainID (security #3): SaveCheckpoint wrapped in BEGIN IMMEDIATE transaction. Pre-check of existing chain_id under the write lock rejects cross-chain TaskID collisions before the INSERT OR REPLACE clobbers anything. 7. CheckTx-rejection now deletes the checkpoint: Stale STALEHASH checkpoint after CheckTx failure was an audit- confusing dangling record. Deleting it leaves the retry path clean: LoadCheckpoint returns nil, fall through to re-sign at unchanged sequence on the next attempt. 8. OpenAPI bearerAuth on all mutating + read endpoints: Previously declared only on POST /v0/tasks. GET (list/get) and DELETE now also require bearerAuth. Code-gen clients see the correct contract regardless of which endpoint they target. Tests: - TestInclusionPollingTimeout_* renamed and split into TestInclusionPollingCtxCancel_ReturnsCtxErr (asserts ctx-cancel surfaces as error) and TestInclusionPollingDeadline_ReturnsNonTerminal (asserts poll-deadline-without-ctx-cancel is non-Terminal success with IncludedAt=nil). Uses var-overridable inclusionPollTimeout for speed instead of injecting a ctx deadline. Deferred (tracked separately, not blocking this PR): - k8s B2: controller#222 needs to inject SEI_KEYRING_BACKEND and SEI_KEYRING_DIR alongside SEI_KEYRING_PASSPHRASE — flagged on #222. - security #4: signTxClientFactory sync guard (parallel-tests race) — follow-up; current t.Cleanup pattern works for sequential. - security #10: polling vs mempool ambiguity (use /unconfirmed_txs to distinguish 'still in mempool' from 'never broadcast') — follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add common UCI workflows to lint, stale check and version.