Repository navigation
fix(ralph): stop environment noise from faking omc ralph verify failures - #4839
0DennisLee0 wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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.
Reviewed commit: 15c9d25fb5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (/^(?:\[[^\]]*\]\s*)*(?:<v>\s+)?(?:TRACE|DEBUG|INFO|NOTICE)\b/i.test(line) | ||
| || /^\[(?:TRACE|DEBUG|INFO|NOTICE)\]/i.test(line)) continue; |
There was a problem hiding this comment.
When a configured feedback command changes from success to a nonzero run whose only diagnostic is INFO validation failed, both the baseline and rerun normalize to an empty signature set here. runFeedbackCommand does not encode ordinary nonzero statuses and diffAgainstBaseline sees only signatures, so omc ralph verify returns 0 and lets Ralph proceed with a failed check; retain failure-bearing low-level messages or record the exit-status transition.
AGENTS.md reference: src/hooks/AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a8d19b9: CommandBaseline now records exitCode, and diffAgainstBaseline treats a 0 → nonzero transition as a new failure (the reverse as resolved). An INFO-only failing run is therefore caught by its exit status even though its signatures are filtered out. Added transition regression tests.
| if (/^>\s+\S+@\S+/.test(line)) continue; | ||
| if (/^>\s+(?:tsc|vite|vitest|jest|node|npm|yarn|pnpm|eslint|tsx|ts-node)\b/i.test(line)) continue; | ||
| if (/^(?:RUN\s+v?\d|Start at\s|Duration\s)/i.test(line)) continue; | ||
| if (/^(?:Test Files|Tests)\b/.test(line) && !/\bfailed\b/i.test(line)) continue; |
There was a problem hiding this comment.
failed
When a quiet feedback runner exits nonzero with a summary such as Tests: 2 failures or Tests: 1 error, this condition treats it as a green summary because it only exempts the literal word failed. Because ordinary nonzero status is absent from CommandBaseline, the current signature set can remain empty and omc ralph verify reports clean; only drop summaries proven to contain passed/skipped counts, or recognize failure/error/failing variants.
AGENTS.md reference: src/hooks/AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a8d19b9: the green-summary filter now matches every failure variant (fail/failed/failure/failures/failing/error/errors), so Tests: 2 failures and Tests: 1 error are retained. Added regression coverage.
Review Report: PR #4839✓ Tests PassAll 25 tests in ✓ Type SafetyTypeScript: 0 errors via Verified Filter BehaviorReproduction of described noise verified in automated tests:
Finding: Target Branch MismatchSeverity: P1 Recommendation: Author should retarget the PR base to Finding: Generated Artifact Digest MismatchSeverity: P2 (Non-blocking for code review) This indicates a packaging/generated-artifact state problem, not a code issue. The check will pass once the PR is retargeted to Code Quality: No Issues Found
Acceptance: ✓ Implementation CompleteAll concrete changes verified:
Action Required: Retarget PR base to — |
|
Note on the review report above: it was run against — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8d19b975a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!line || line.length > 500) continue; | ||
| // Environment noise: below-WARN log levels, package-manager banners and | ||
| // warnings, and runner headers/summaries carry no failure signal. | ||
| if (/^(?:\[[^\]]*\]\s*)*(?:<v>\s+)?(?:TRACE|DEBUG|INFO|NOTICE)\b/i.test(line) |
There was a problem hiding this comment.
When a feedback command already exits nonzero at baseline (the supported pre-existing-failure case) and a later regression's only distinguishing diagnostic is INFO validation failed, this blanket filter removes that diagnostic from both sets. The newly added exit-code check only recognizes a 0→nonzero transition, so a 1→1 rerun has neither a new status nor a new signature and omc ralph verify returns 0; retain low-level lines that contain failure/error terms.
AGENTS.md reference: src/hooks/AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 74687dc8f: below-WARN filtering now keeps lines carrying failure vocabulary (INFO validation failed) instead of blanket-dropping them, so a 1 → 1 rerun can still surface the new diagnostic. The exit-code transition check still covers 0 → nonzero.
| // often prefixed with /private. Left unnormalized, every rerun's random | ||
| // directory (pmd_scan_80gbSQ, scr-unit-repo-0yogcC) fingerprints as a new | ||
| // failure even though the run is identical. | ||
| /(?:\/private)?\/var\/folders\/[^\s"']*/g, |
There was a problem hiding this comment.
When a command already has a tolerated nonzero baseline and emits failures from a macOS temp checkout, this regexp consumes the entire path, including the stable repo-relative filename and line/column: for example, failures in .../repo-A/src/a.ts:1:1 and .../repo-B/src/b.ts:1:1 with the same message both become <v>. Those failures then deduplicate while the exit status remains nonzero, allowing omc ralph verify to report clean; normalize only the volatile temp-root component and retain the suffix.
AGENTS.md reference: src/hooks/AGENTS.md:L148-L151
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 74687dc8f: macOS temp paths now normalize to <v> while retaining the trailing filename/line (<v>/pmd.sarif, <v>/a.ts:1:1), so different failure locations stay distinct. Added regression tests for nested stable subdirs and different line numbers.
`omc ralph verify` fingerprints every non-progress output line, so volatile
environment output was recorded as failure signatures and every rerun of an
unchanged tree exited 1:
- macOS `os.tmpdir()` paths (/var/folders/<xx>/<hash>/T/<random>)
- `Start at` clock headers, npm banners/warnings, and runner chrome
- below-WARN INFO logs and passing `✓` cases (a passing test named
`EmptyDiffError` bypassed the old `fail|error` exception)
Normalize macOS temp paths to `<v>` while keeping the trailing filename and
line/column (`<v>/pmd.sarif`, `<v>/a.ts:1:1`) so different failures stay
distinct. Drop below-WARN logs unless the message reports a failure
("INFO validation failed"), and recognize every failure-summary variant
(fail/failed/failure/failures/failing/error/errors) instead of only
"failed".
Record each feedback command's exit status in the baseline: a 0 -> nonzero
transition is a new failure even when the only diagnostic is filtered out
as noise, and the reverse is reported as resolved.
Existing baselines must be re-recorded with --write-baseline after
upgrading: old entries contain unnormalized paths and no exit status.
a8d19b9 to
74687dc
Compare
Code Review: PR #4839Reviewed Head Commit: Verification Summary:
Previous Codex Findings - All ResolvedCommit a8d19b9 (P1: Preserve failure-bearing INFO output)
Commit 15c9d25 (P2: Retain failure summaries that do not use 'failed')
Commit a8d19b9 (P1: Preserve failure-bearing INFO without successful baseline)
Commit a8d19b9 (P2: Preserve stable suffixes in macOS temp paths)
Adversarial Filter VerificationConfirmed that filters do NOT suppress real failures (10 tests):
Test ResultsFiles Changed
CI Status
Blocking FindingsNone. All P1/P2 Codex findings fixed and tested. [repo owner's gaebal-gajae (clawdbot) 🦞] |
|
Closed and reopened only to re-trigger the generated-artifact check: its 11:02:11Z run started 4s before the base moved from main to dev, so it verified against the main base. No code change. @codex review — the last Codex run on — |
💡 Codex Reviewoh-my-claudecode/src/hooks/ralph/feedback-baseline.ts Lines 188 to 189 in 74687dc Check exit status for commands absent from the baseline
If a story adds an auto-detected package script after the baseline is recorded, AGENTS.md reference: src/hooks/AGENTS.md:L148-L151 Retain exception-class diagnostics in low-level logs
For a feedback command that already exits nonzero at baseline, the new word-boundary matcher drops diagnostics such as AGENTS.md reference: src/hooks/AGENTS.md:L148-L151 oh-my-claudecode/src/hooks/ralph/feedback-baseline.ts Lines 61 to 62 in 74687dc Preserve stable directories in normalized macOS paths
For a command with a tolerated nonzero baseline on macOS, failures at AGENTS.md reference: src/hooks/AGENTS.md:L148-L151 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Codex Findings Verification — Previous "No New Findings" Verdict SupersededOur 11:10Z comment declared this PR "no new Codex findings" on head 74687dc. At 11:23:35Z, Codex posted two NEW findings on the same commit that are confirmed as real regressions. I've reproduced both with throwaway vitest cases: P1 CONFIRMED: "Check exit status for commands absent from the baseline" (L188-189)Issue: When a command is new (not in the baseline) and exits with code 1 but produces no failure signatures (e.g., only wrapper banners), Repro on HEAD: const current = { 'auto-script': { signatures: [], exitCode: 1 } };
const diff = diffAgainstBaseline(null, current); // null baseline = new command
// Expected: diff.newSignatures includes '<exited with code 1>'
// Actual: diff.newSignatures is [] ← FAILSRoot cause: Lines 179-181 only add signatures for new commands: if (!before) {
newSignatures.push(...entry.signatures.map(...));
continue; // ← skips exitCode check on line 188
}The exit-code logic on lines 188-189 only runs when Fix: After line 180, add exit-code check for new commands: if (!before) {
newSignatures.push(...entry.signatures.map(...));
if (entry.exitCode !== undefined && entry.exitCode !== 0) {
newSignatures.push(`${command}: <exited with code ${entry.exitCode}>`);
}
continue;
}Not mergeable until P1 is fixed with a test covering: new command with P2 CONFIRMED: "Retain exception-class diagnostics in low-level logs" (L26)Issue: FAILURE_WORD_PATTERN uses word boundaries:
When these appear in INFO/DEBUG log lines (e.g., Repro on HEAD: signatureLines('INFO TypeError: Cannot read property "x"');
// Expected: returns ['INFO TypeError: Cannot read property "x"']
// Actual: returns [] ← FAILS, line dropped by log-level filterRoot cause: The new log-level filter (line 89-90): if ((/^(?:\[[^\]]*\]\s*)*(?:<v>\s+)?(?:TRACE|DEBUG|INFO|NOTICE)\b/i.test(line)
|| /^\[(?:TRACE|DEBUG|INFO|NOTICE)\]/i.test(line)) && !FAILURE_WORD_PATTERN.test(line)) continue;Combined with FAILURE_WORD_PATTERN that requires word boundaries, exception class names are dropped. Old behavior (dev): Used Fix: Update FAILURE_WORD_PATTERN to match exception types: const FAILURE_WORD_PATTERN = /(?:fail(?:ed|ure|ures|ing)?|\w*Error\b)|\w*Exception\b|✗|×/i;This adds Summary
Merge readiness: Not mergeable. P1 must be fixed with a passing test before approval. — |
Problem
omc ralph verifycan never report clean in a project whose feedback command prints volatile environment output:omc ralph verify --write-baselinenewFailuresOne observed session produced 65 new + 66 resolved signatures, entirely noise:
lefthookwarnings containing a random temp repo pathINFOlog lines containing a randompmd_scan_<id>temp dirStart at HH:MM:SSheaderRoot cause
signatureLines()(src/hooks/ralph/feedback-baseline.ts) fingerprints every non-progress output line as a failure signature, anddiffAgainstBaseline()compares the sets exactly. Two gaps compound:os.tmpdir()paths (/var/folders/.../T/<random>, often/private/var/folders/...) are not normalized./tmp/...was covered, but that is not where macOS puts temp dirs.INFOlogs, npm banners and warnings, runner headers, and even passing✓cases are recorded. A passing case whose name containsError(e.g.EmptyDiffError) bypassed the existingfail|errorexception and was stored as a failure signature.Because the random temp names differ on every run, each rerun structurally exits 1 regardless of real regressions.
Changes
<v>, keeping the trailing filename and line/column (<v>/pmd.sarif,<v>/a.ts:1:1) so different failures stay distinctTRACE/DEBUG/INFO/NOTICEunless the message itself reports a failure (INFO validation failed), plus npm banners/warnings,RUN/Durationheaders, greenTest Files/Testssummaries, and source-context frames✓/√lines as passing unconditionally; keep✗/×andFAILlines0 → nonzerotransition is a new failure even when its only diagnostic is filtered as noise; the reverse is reported as resolvedfail/failed/failure/failures/failing/error/errors)Validated against a real 57KB
npm testlog: extracted signatures drop from 117 to 17, while the genuineENOTEMPTYfailure and itsFAILline are still present.Review feedback addressed
CommandBaselinenow recordsexitCode, anddiffAgainstBaselinetreats a0 → nonzerotransition as a new failure. Originally this covered a failing run whose only diagnostic was filtered out.failed(P2):Tests: 2 failures/Tests: 1 errorwere dropped; the filter now recognizes all failure variants.1 → 1rerun can still surface a newINFO validation failed.<v>; the trailing filename/line is retained, soa.ts:1:1andb.ts:2:2stay distinct.Migration
Re-record the baseline once after upgrading:
Old baselines contain unnormalized paths and no exit status, so the first run against them reports one-time noise.
Verification
npx vitest run src/hooks/ralph/__tests__/feedback-baseline.test.ts→ 55 passednpx eslint <changed files>→ cleannpx tsc --noEmit→ cleanexitCode 0; a rerun reportednewFailures: [],resolvedFailures: [], exit 0 — the original setup exited 1 with 65 false new failures./var/folders/.../T/...paths andStart atstamps: baseline + two reruns all exit 0,clean vs baseline.npx vitest run src/cli/__tests__/ralph-afk.test.tshas one pre-existing macOS-only failure (os.tmpdir()returns/var/...whileprocess.cwd()resolves to/private/var/...). It fails identically on the base branch with this patch stashed.PR is based on
devper the repository convention, and the branch is rebased onto the currentdev(which already carries the pytest and Node-warning normalization work).