fix(qa): accept documented post-create diagnostics - #785
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe smoke validator now requires four core post-create reason codes, permits defined prepared-card diagnostics, normalizes camelCase and snake_case aliases, and validates diagnostic consistency. Tests add split-alias fixtures and prepared-state conflict scenarios. ChangesPost-create proof validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
evaOS review status: stale headPR: #785 - fix(qa): accept documented post-create diagnostics evaOS review stopped because this queued head is no longer the live PR head. Automation note: agents should wait for this comment to reach PR URL: #785 Details: Superseded by a newer PR head. |
8dad2cc to
ad7ff37
Compare
|
@neondiff re-review |
evaOS review status: completedPR: #785 - fix(qa): accept documented post-create diagnostics evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #785 Review URL: #785 (review) |
|
evaOS Code Review Bot queued a current-head review for Command: Safety boundary: command handling cannot approve, merge, repair, push branches, or expand repo permissions. |
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #785 - fix(qa): accept documented post-create diagnostics
Head: 7a1c5e5b48ab468e8fcd0171bce7698e37ea29c6 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 1/5 (~14 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
packages/cli/src/openclaw-tool-smoke.ts |
modified | +54/-7 | Changed file | Low |
tests/openclaw-tool-smoke.test.ts |
modified | +46/-2 | Test coverage | Low |
Review Signal
No validated inline findings.
Dropped findings before posting: 0. High-severity findings: 0.
Risk Taxonomy
No finding categories.
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: TypeScript/web build or CI proof - Runtime TypeScript/web files or package/config files changed. Proof: npm run build; typecheck; focused Vitest; green GitHub check.
Proof status: sufficient - PR metadata mentions acceptable proof for each required validation recommendation.
Profile validation hints: Call out evidence leakage, replay/collision risks, duplicate side effects, and brittle sanitizer logic.
Profile proof expectations: Look for focused sanitizer, signature, orchestration, or fixture proof.
Related Context
Related issues/PRs: #784.
Suggested labels: tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions: none
- Label suggestions: orchestration, security, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
codex
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/openclaw-tool-smoke.ts (1)
1842-1947: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftHigh-complexity validator: consider extracting helpers and differentiating blocker codes.
successfulPostCreateProofBlockersnow packs alias normalization for bothpreparedStateandreasonCodes, plus cross-field consistency checks, into one ~90-line function (flagged by static analysis as high complexity). Two concrete improvements:
- Extract
normalizePreparedState(proof)andnormalizeReasonCodeAliases(value)helpers so the alias-reconciliation logic (Lines 1871-1907) is independently testable/readable.- All failure modes (missing required code, unknown code, alias mismatch, prepared-state/reason-code incoherence) collapse into a single
post_create_proof_reason_codes_invalidblocker (Line 1939). Given this exact ambiguity class already caused a P1 regression, splitting into distinct blocker strings (e.g...._alias_mismatch,..._prepared_state_inconsistent) would make future failures far faster to triage from CI output alone.♻️ Sketch of extracted helper
function normalizePreparedState(proof: Record<string, unknown>): { available: boolean; current: boolean } | null | undefined { const aliases = [proof.preparedState, proof.prepared_state].filter((entry) => entry !== undefined); if (aliases.length === 0) return undefined; const normalized = aliases.filter(isRecord).map((state) => { const availableAliases = [state.cardAvailable, state.card_available].filter((entry) => entry !== undefined); const currentAliases = [state.cardCurrent, state.card_current].filter((entry) => entry !== undefined); const valid = availableAliases.length > 0 && currentAliases.length > 0 && availableAliases.every((entry) => typeof entry === "boolean" && entry === availableAliases[0]) && currentAliases.every((entry) => typeof entry === "boolean" && entry === currentAliases[0]); return valid ? { available: availableAliases[0] === true, current: currentAliases[0] === true } : null; }); const valid = normalized.length === aliases.length && normalized.every((entry) => entry !== null && JSON.stringify(entry) === JSON.stringify(normalized[0])); return valid ? normalized[0] : null; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/openclaw-tool-smoke.ts` around lines 1842 - 1947, Refactor successfulPostCreateProofBlockers by extracting the preparedState alias reconciliation into normalizePreparedState(proof) and the reasonCodes alias normalization into normalizeReasonCodeAliases(value), preserving current validation behavior. Replace the single post_create_proof_reason_codes_invalid result with distinct blocker codes for reason-code alias mismatches, missing or unknown codes, and prepared-state/reason-code inconsistency so CI identifies the precise failure category.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/cli/src/openclaw-tool-smoke.ts`:
- Around line 1842-1947: Refactor successfulPostCreateProofBlockers by
extracting the preparedState alias reconciliation into
normalizePreparedState(proof) and the reasonCodes alias normalization into
normalizeReasonCodeAliases(value), preserving current validation behavior.
Replace the single post_create_proof_reason_codes_invalid result with distinct
blocker codes for reason-code alias mismatches, missing or unknown codes, and
prepared-state/reason-code inconsistency so CI identifies the precise failure
category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad8ec05e-88dd-48ae-9cdc-76f398ac9806
📒 Files selected for processing (2)
packages/cli/src/openclaw-tool-smoke.tstests/openclaw-tool-smoke.test.ts
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
packages/cli/src/openclaw-tool-smoke.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (5)
packages/cli/src/openclaw-tool-smoke.ts (1)
140-151: 🗄️ Data Integrity & IntegrationVerify
prepared_card_missingis an intentionally-permitted supplemental code.Issue
#784's objective text names onlyprepared_card_availableandprepared_card_stale_or_not_readyas the permitted supplemental codes, butALLOWED_SUCCESSFUL_POST_CREATE_REASON_CODESalso allowsprepared_card_missing. The validator logic and test fixtures (e.g. the "no-card branch" success case) rely onprepared_card_missingbeing present, so the implementation appears internally consistent — but please confirm with the actual gateway/product contract that a third supplemental code is genuinely part of the documented output, since this constant is the exact boundary this P1 fix is meant to lock down.tests/openclaw-tool-smoke.test.ts (4)
116-124: LGTM!
531-537: Fixture correctly exercises the new alias-conflict branches.The nested
conflictingSuccessfulPostCreatePreparedStateObjectsbranch and updated camelCase/reason-code fixture both correctlyprocess.exit(0)before falling through, and the emitted shapes match whatsuccessfulPostCreateProofBlockersexpects to reject/accept.
1417-1417: LGTM!
1447-1478: 🎯 Functional CorrectnessConfirm a positive dual-alias success case exists elsewhere.
This matrix thoroughly covers rejection of conflicting/incoherent prepared-state and reason-code combinations, but all visible success fixtures (
camelCaseSuccessfulPostCreateProof, default snake_case) exercise only a single alias family at a time. Since this PR's stated goal is matching "the real persisted response" — which per the alias-normalization logic could legitimately include bothpreparedState/prepared_stateandreasonCodes/reason_codessimultaneously and agreeing — please confirm a positive test exists (outside this diff) asserting acceptance when both aliases are present and consistent, not just rejection when they conflict.
evaOS review status: closed or merged before reviewPR: #785 - fix(qa): accept documented post-create diagnostics evaOS review stopped because the PR closed or merged before this queued head could be reviewed. Automation note: agents should wait for this comment to reach PR URL: #785 |
|
Current-head release-gate disposition for
The PR is merge-ready for the 1.6 release train. Publication and active Eva upgrade remain separate post-merge gates. |
1 participant
Closes #784
Summary
Validation
npm ci(production build passed)node --test --import tsx tests/openclaw-tool-smoke.test.ts— 89/89 passednpm run typecheckwas also probed but is not the canonical gate and currently reports pre-existing Node 26 test-typing errors across unrelated files; no errors point to the changed filesRelease evidence
The exact 1.6.0 candidate passed 67/68 full gateway invocations. The sole failure was the validator rejecting the live product’s six valid reason codes (four core plus
prepared_card_availableandprepared_card_stale_or_not_ready). After merge, the exact package/full matrix will be rebuilt and rerun before publication.Summary by CodeRabbit