fix(#485): typed sender contract + notification incident coalescing - #522
Merged
richard-devbot merged 2 commits intoJul 31, 2026
Merged
Conversation
…or the validate path
Confirmed and fixed the audit's exact false-positive-delivery finding:
notifyAll() only treated a THROWN sender error as ok:false — sendEmail's
deliberate "never throws" contract returned bare status strings on every
failure path, all of which notifyAll recorded as delivered. sendEmail now
returns { ok, message } and notifyAll interprets it directly; every other
channel's plain-string-on-success shape is unaffected. Found and fixed the
same bug class one level lower: the Telegram Bot API can return HTTP 200
with a body-level failure, which postJson's status-code-only check missed —
sendTelegram now parses the body and throws (assertTelegramBodySucceeded,
directly unit-tested since sendTelegram hardcodes the real API host).
Confirmed the audit's math against the real code: sdlc_validate
unconditionally fired two messages (stage + report) on every call
regardless of PASS/FAIL, plus a third on exhaustion, with zero memory of
prior attempts on the same task. src/notifications/incidents.js is a
bounded incident-coalescing state machine: one incident per task's current
unbroken failure streak, persisted per run. A FAIL on an already-open
incident suppresses the external notification (a notification_coalesced
event still records it); exhaustion escalates the same incident exactly
once; a later PASS resolves and clears it. Every dispatch attempt is
recorded via a new notification_dispatched event with per-channel {channel,
ok} — directly serving "per-channel delivery status is accurate and
queryable" without requiring real network access to test the wiring.
Also closed "malformed routing configuration can fail silently": a
syntactically-invalid or wrong-shape notifications.json/policy.json used to
degrade to {}/null with zero signal. Both now console.error at the point of
the actual runtime degrade.
Deferred (documented in docs/HARNESS.md): the full transactional
notification outbox tied to #481's ledger, durable retry/backoff, a
dead-letter queue, rate bounds/quiet hours, and a Business Hub incident UI —
none of these exist anywhere in this codebase; building them is a separate,
much larger subsystem than this slice, comparable in size to the #481
attempt-ledger build itself.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds persisted validation-incident coalescing, structured email results, Telegram body-level failure detection, accurate dispatch events, and runtime diagnostics for malformed notification configuration. It adds unit, integration, and channel tests and documents the behavior. ChangesNotification incident lifecycle
Notification delivery contracts
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix notification delivery accounting and coalesce validate-path incidents (#485)
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+140
to
+143
| const env = { | ||
| RSTACK_SLACK_WEBHOOK: 'https://hooks.slack.com/x', | ||
| RSTACK_ACS_CONNECTION_STRING: 'endpoint=https://contoso.communication.azure.com;accesskey=YQ==', | ||
| }; |
There was a problem hiding this comment.
1. Hardcoded connection strings in tests 📜 Skill insight ⛨ Security
Test code introduces hardcoded webhook/connection-string-like values (e.g., Slack webhook URL and ACS accesskey= connection string), which can normalize committing credential-shaped secrets. Use environment variables or explicit fake fixtures to avoid accidental real secret leakage.
Agent Prompt
## Issue description
Tests include hardcoded strings that look like real secrets/credentials (Slack webhook URL and Azure Communication Services connection string with `accesskey=`). This violates the requirement that test files must not contain hardcoded secrets.
## Issue Context
Even if these are placeholders, committing credential-shaped values increases the risk that real tokens get committed later by copy/paste.
## Fix Focus Areas
- tests/notifications-channels.test.js[140-143]
- tests/email-approvals-353.test.js[203-209]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // still records it, so nothing is silently dropped from the | ||
| // ledger) — only the FIRST failure, the exhaustion escalation, | ||
| // and the eventual recovery notify externally. | ||
| const notificationOutcome = status === "PASS" ? "PASS" : retryDecision?.action === "exhausted" ? "EXHAUSTED" : "FAIL"; |
There was a problem hiding this comment.
2. Nested ternary for outcome 📜 Skill insight ⚙ Maintainability
The new notificationOutcome assignment uses a nested ternary, which is harder to read and easier to misinterpret during incident/notification logic changes. Refactor to explicit conditional logic for clarity.
Agent Prompt
## Issue description
A nested ternary is used to compute `notificationOutcome`, which makes the control flow less explicit in a critical notification/incident path.
## Issue Context
This code controls when notifications are opened/escalated/resolved/suppressed. Readability matters to avoid logic mistakes.
## Fix Focus Areas
- src/integrations/pi/rstack-sdlc.ts[3615-3615]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+46
to
+57
| export function loadIncidentState(runDir) { | ||
| const path = incidentsPath(runDir); | ||
| if (!existsSync(path)) return {}; | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(path, 'utf8')); | ||
| return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed : {}; | ||
| } catch { | ||
| // A corrupt incidents file must never block validation — degrade to no | ||
| // known incidents (worst case: one extra notification is sent, never a | ||
| // dropped one). | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
3. loadincidentstate error path untested 📜 Skill insight ▣ Testability
loadIncidentState() contains a new JSON-parse error-handling path that silently degrades to {}
on corrupt incident state files, but the added tests do not trigger or assert this behavior. This
violates the requirement that newly added error-handling paths have tests covering the error
condition.
Agent Prompt
## Issue description
`loadIncidentState()` has a catch-based error path for corrupt JSON, but tests only cover the missing-file path.
## Issue Context
Per compliance, newly added error-handling branches should have tests that trigger the error condition and assert the expected behavior.
## Fix Focus Areas
- src/notifications/incidents.js[46-57]
- tests/notifications-incidents-485.test.js[100-107]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+112
| /** | ||
| * Decide what to do about a task's outcome given its current incident state. | ||
| * Pure — takes the loaded state object, returns { action, nextState } and | ||
| * never mutates its input. Caller persists nextState and acts on `action`. | ||
| * | ||
| * outcome: 'FAIL' | 'PASS' | 'EXHAUSTED' | ||
| * - FAIL, no open incident → action 'open' (notify — a new failure condition) | ||
| * - FAIL, incident open → action 'update' (suppress — already known) | ||
| * - EXHAUSTED → action 'escalate' (notify exactly once — the | ||
| * escalation IS the notification; a caller only reaches this once per | ||
| * incident since sdlc_validate's retry policy stops retrying after it) | ||
| * - PASS, incident was open → action 'resolve' (notify — recovery) | ||
| * - PASS, no incident → action 'notify' (routine informational ping — | ||
| * nothing to coalesce; unchanged frequency, but see the router-side fix | ||
| * that collapses the old double-message send into one) | ||
| */ | ||
| export function evaluateNotificationIntent(state, incidentKey, outcome, now = new Date().toISOString()) { | ||
| const existing = state[incidentKey] ?? null; | ||
| if (outcome === 'FAIL') { | ||
| if (!existing) { | ||
| return { action: 'open', nextState: { ...state, [incidentKey]: { status: 'open', opened_at: now, attempts: 1 } } }; | ||
| } | ||
| return { action: 'update', nextState: { ...state, [incidentKey]: { ...existing, attempts: (existing.attempts ?? 1) + 1 } } }; | ||
| } | ||
| if (outcome === 'EXHAUSTED') { | ||
| const base = existing ?? { status: 'open', opened_at: now, attempts: 1 }; | ||
| return { action: 'escalate', nextState: { ...state, [incidentKey]: { ...base, status: 'escalated' } } }; | ||
| } | ||
| if (outcome === 'PASS') { | ||
| if (existing) { | ||
| const { [incidentKey]: _removed, ...rest } = state; | ||
| return { action: existing.status === 'escalated' ? 'resolve_from_escalation' : 'resolve', nextState: rest }; | ||
| } | ||
| return { action: 'notify', nextState: state }; | ||
| } | ||
| throw new Error(`evaluateNotificationIntent: unknown outcome "${outcome}"`); | ||
| } | ||
|
|
||
| /** | ||
| * Read-decide-write under the run's own file lock, so two concurrent | ||
| * validate calls on different tasks (independent incidentKeys) never race | ||
| * each other's read-modify-write of the shared incidents file. | ||
| */ | ||
| export async function applyNotificationIntent(runDir, incidentKey, outcome) { | ||
| const path = incidentsPath(runDir); | ||
| let result; | ||
| await withFileLock(path, async () => { | ||
| const state = loadIncidentState(runDir); | ||
| result = evaluateNotificationIntent(state, incidentKey, outcome); | ||
| await writeFileAtomic(path, JSON.stringify(result.nextState, null, 2)); | ||
| }); | ||
| return result.action; | ||
| } |
There was a problem hiding this comment.
4. evaluatenotificationintent missing jsdoc tags 📜 Skill insight ⚙ Maintainability
Newly exported functions in src/notifications/incidents.js do not include JSDoc with @param, @returns, and @throws as required. This reduces API clarity for callers and violates the exported-function documentation requirement.
Agent Prompt
## Issue description
Exported functions in `src/notifications/incidents.js` are missing required JSDoc tags (`@param`, `@returns`, `@throws`).
## Issue Context
The compliance rule requires exported/public functions to have complete JSDoc to keep contracts explicit.
## Fix Focus Areas
- src/notifications/incidents.js[38-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+57
to
+140
| test('#485: three failed validation attempts open one incident and dispatch far fewer than six notifications', async (t) => { | ||
| const projectRoot = mkdtempSync(join(tmpdir(), 'rstack-485-coalesce-')); | ||
| t.after(() => { | ||
| process.env.RSTACK_PROJECT_ROOT = undefined; | ||
| process.env.RSTACK_SLACK_WEBHOOK = undefined; | ||
| rmSync(projectRoot, { recursive: true, force: true }); | ||
| }); | ||
| process.env.RSTACK_PROJECT_ROOT = projectRoot; | ||
| process.env.RSTACK_SLACK_WEBHOOK = `https://127.0.0.1:${await unreachablePort()}/webhook`; | ||
|
|
||
| mkdirSync(join(projectRoot, '.rstack'), { recursive: true }); | ||
| // Raise the attempt budget to 3 so this test can exercise the audit's own | ||
| // literal worked example ("three failed attempts... six standalone | ||
| // alerts") rather than the default budget of 2. | ||
| writeFileSync(join(projectRoot, '.rstack', 'rstack.config.json'), JSON.stringify({ | ||
| guardrails: { maxTaskAttempts: 3 }, | ||
| })); | ||
|
|
||
| extension(mockPi); | ||
| const start = await mockPi.tools.sdlc_start.execute('1', { goal: 'Notification coalescing wiring check' }); | ||
| const runId = start.details.run_id; | ||
| const runDir = join(projectRoot, '.rstack', 'runs', runId); | ||
|
|
||
| await mockPi.tools.sdlc_plan.execute('2', { run_id: runId }); | ||
| const firstTaskId = readJson(join(runDir, 'tasks.json')).tasks[0].id; | ||
| await mockPi.tools.sdlc_approve.execute('3', { run_id: runId, artifact: 'plan.md', status: 'APPROVED' }); | ||
| await mockPi.tools.sdlc_build_next.execute('4', { run_id: runId }); | ||
|
|
||
| const dispatched = () => readEvents(runDir).filter((event) => event.type === 'notification_dispatched'); | ||
| const coalesced = () => readEvents(runDir).filter((event) => event.type === 'notification_coalesced'); | ||
| const incidentsPath = join(runDir, 'notification-incidents.json'); | ||
|
|
||
| // Attempt 1: FAIL (no builder.json written) — opens the incident, dispatches one notification attempt. | ||
| const first = await mockPi.tools.sdlc_validate.execute('5', { run_id: runId, task_id: firstTaskId }); | ||
| assert.equal(first.details.status, 'FAIL'); | ||
| assert.equal(dispatched().length, 1, 'attempt 1 opens the incident: exactly one dispatch attempt'); | ||
| assert.equal(dispatched()[0].action, 'open'); | ||
| assert.equal(dispatched()[0].outcome, 'FAIL'); | ||
| assert.equal(coalesced().length, 0); | ||
|
|
||
| assert.ok(existsSync(incidentsPath), 'incident state is persisted'); | ||
| let incidents = readJson(incidentsPath); | ||
| assert.equal(incidents[`task:${firstTaskId}`].status, 'open'); | ||
| assert.equal(incidents[`task:${firstTaskId}`].attempts, 1); | ||
|
|
||
| // Attempt 2: FAIL again (still under the raised budget of 3) — the SAME | ||
| // incident, dispatch suppressed. | ||
| appendFileSync(join(runDir, 'events.jsonl'), `${JSON.stringify({ ts: new Date().toISOString(), type: 'task_started', task_id: firstTaskId })}\n`); | ||
| claimTaskForTest(projectRoot, runId, firstTaskId, { attempt: 2 }); | ||
| const second = await mockPi.tools.sdlc_validate.execute('6', { run_id: runId, task_id: firstTaskId }); | ||
| assert.equal(second.details.status, 'FAIL'); | ||
| assert.equal(dispatched().length, 1, 'attempt 2 on the SAME open incident dispatches NO additional notification'); | ||
| assert.equal(coalesced().length, 1, 'the suppressed attempt is still recorded internally — nothing silently dropped from the ledger'); | ||
| assert.equal(coalesced()[0].task_id, firstTaskId); | ||
| assert.equal(coalesced()[0].outcome, 'FAIL'); | ||
|
|
||
| incidents = readJson(incidentsPath); | ||
| assert.equal(incidents[`task:${firstTaskId}`].attempts, 2, 'the incident tracks the attempt count even while suppressed'); | ||
|
|
||
| // Attempt 3: FAIL, now AT the raised budget — exhausted. The pre-existing | ||
| // approval_required ping fires (unchanged, a distinct call-to-action), plus | ||
| // exactly one escalation dispatch for the incident (not a fresh open). | ||
| appendFileSync(join(runDir, 'events.jsonl'), `${JSON.stringify({ ts: new Date().toISOString(), type: 'task_started', task_id: firstTaskId })}\n`); | ||
| claimTaskForTest(projectRoot, runId, firstTaskId, { attempt: 3 }); | ||
| const third = await mockPi.tools.sdlc_validate.execute('7', { run_id: runId, task_id: firstTaskId }); | ||
| assert.equal(third.details.status, 'FAIL'); | ||
|
|
||
| const tasks = readJson(join(runDir, 'tasks.json')).tasks; | ||
| assert.equal(tasks.find((task) => task.id === firstTaskId).status, 'BLOCKED', 'exhaustion transitions the task to BLOCKED'); | ||
|
|
||
| // Total dispatch attempts across 3 failed attempts: open(1) + suppressed(0) + escalate(1) = 2 | ||
| // via the incident-coalescing path — plus the pre-existing, separate | ||
| // approval_required ping fired by the exhaustion branch (unrelated to this | ||
| // engine, unchanged). The audit's own literal wording for this scenario: | ||
| // "three failed attempts... rather than six standalone alerts" — 2 is far | ||
| // fewer than 6. | ||
| assert.equal(dispatched().length, 2, 'three failed attempts produce exactly 2 coalesced dispatch attempts (open + escalate), not 3+'); | ||
| assert.equal(dispatched()[1].action, 'escalate'); | ||
| assert.equal(coalesced().length, 1, 'still exactly one coalesced (suppressed) event — no new suppressions beyond attempt 2'); | ||
|
|
||
| incidents = readJson(incidentsPath); | ||
| assert.equal(incidents[`task:${firstTaskId}`].status, 'escalated'); | ||
| assert.equal(incidents[`task:${firstTaskId}`].attempts, 2, 'escalation preserves the attempt count accumulated while the incident was open'); | ||
| }, { timeout: 30_000 }); |
There was a problem hiding this comment.
5. Coalescing test lacks aaa 📜 Skill insight ▣ Testability
The new coalescing integration test interleaves setup, execution, and assertions without clear Arrange/Act/Assert separation, which makes the test harder to maintain and reason about. This violates the required AAA structure for tests in this repo.
Agent Prompt
## Issue description
The test body mixes setup, multiple actions, and assertions without clear Arrange/Act/Assert structure.
## Issue Context
Compliance requires tests to follow the Arrange-Act-Assert (AAA) pattern.
## Fix Focus Areas
- tests/extension-notification-coalescing-485.test.js[57-140]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+140
to
+141
| }, { timeout: 30_000 }); | ||
|
|
There was a problem hiding this comment.
6. 30_000 timeout magic number 📜 Skill insight ⚙ Maintainability
The new tests introduce a hardcoded timeout: 30_000 literal instead of a named constant, which reduces clarity and makes future adjustments error-prone. Magic numbers should be replaced with descriptive named constants.
Agent Prompt
## Issue description
A hardcoded numeric literal (`30_000`) is used as a timeout value rather than a named constant.
## Issue Context
Compliance requires replacing magic numbers used for timeouts/config with named constants to improve readability and maintainability.
## Fix Focus Areas
- tests/extension-notification-coalescing-485.test.js[140-141]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/integrations/pi/rstack-sdlc.ts (1)
3623-3630: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider recording per-channel failure detail, not just
ok.
notification_dispatched(line 3661) only records{channel, ok}, droppingresult.detailreturned bynotifyAll. The doc comment above (lines 3650-3655) frames this event as answering "here's what happened" for a dispatch attempt — including the failure detail would make that record actually diagnostic without a separate cross-reference.♻️ Proposed enhancement
- channels: results.map((result) => ({ channel: result.channel, ok: result.ok })), + channels: results.map((result) => ({ channel: result.channel, ok: result.ok, detail: result.detail })),Also applies to: 3649-3662
🤖 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 `@src/integrations/pi/rstack-sdlc.ts` around lines 3623 - 3630, Update the notification_dispatched event construction in the notifyAll dispatch flow to include each channel result’s diagnostic detail alongside channel and ok. Preserve the existing per-channel event emission and use the detail returned by notifyAll directly, including it for failures so the event fully describes the dispatch attempt.
🤖 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.
Inline comments:
In `@src/integrations/pi/rstack-sdlc.ts`:
- Around line 3615-3648: Update the notification fallback in the message
construction around notificationAction and messageByAction so an
applyNotificationIntent failure derives the banner from notificationOutcome
rather than defaulting to the success-oriented "notify" message. Preserve
action-specific banners when notification intent evaluation succeeds, while
ensuring FAIL and EXHAUSTED outcomes use outcome-appropriate messaging
consistent with displayStatus.
- Around line 3617-3622: The notification flow around applyNotificationIntent
must record a compensating pinned dispatch-failure event whenever
buildRunReport, notifyAll, or notification_dispatched fails after an incident is
opened. Separate report-building fallback from dispatch failure handling, and
ensure retries cannot silently return update for an already-open incident
without recording the failed dispatch.
In `@src/notifications/incidents.js`:
- Around line 25-27: Add a schema_version field to the documented incident
record and every new-record construction path in evaluateNotificationIntent,
using one consistent current version value. Update loadIncidentState to preserve
and validate this marker when reading persisted state, rejecting or explicitly
handling records with missing or unsupported versions rather than silently
accepting them.
- Around line 46-58: Update src/notifications/incidents.js lines 46-58 in
loadIncidentState to log the malformed incidents-file error with console.error
before returning {}. Update tests/notifications-incidents-485.test.js lines
100-107 to write invalid JSON to notification-incidents.json and assert
loadIncidentState returns {}, exercising the catch branch and log behavior.
In `@src/notifications/router.js`:
- Around line 52-61: Update the configuration validation and return logic around
the visible channels parsing flow to require a non-null, non-array object for
both the top-level parsed document and parsed.channels; arrays must be rejected
rather than returned or spread by resolveChannels(). Emit the existing
non-blocking diagnostic for malformed top-level and channels configurations,
while preserving {} as the fallback. Add regression tests covering top-level
arrays and channels arrays.
In `@tests/email-approvals-353.test.js`:
- Around line 206-209: Update the malformed-connection assertion in the
sendEmail test to directly verify that bad.message does not exceed the
120-character limit enforced by the email implementation. Remove the
unconditional “|| true” expression while preserving the existing failure-status
and non-throwing assertions.
---
Nitpick comments:
In `@src/integrations/pi/rstack-sdlc.ts`:
- Around line 3623-3630: Update the notification_dispatched event construction
in the notifyAll dispatch flow to include each channel result’s diagnostic
detail alongside channel and ok. Preserve the existing per-channel event
emission and use the detail returned by notifyAll directly, including it for
failures so the event fully describes the dispatch attempt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93269d14-706d-45ea-b77e-3b4505927821
📒 Files selected for processing (13)
docs/HARNESS.mdsrc/integrations/pi/rstack-sdlc.tssrc/notifications/channels/email.jssrc/notifications/channels/telegram.jssrc/notifications/incidents.jssrc/notifications/index.jssrc/notifications/recipients.jssrc/notifications/router.jstests/email-approvals-353.test.jstests/extension-notification-coalescing-485.test.jstests/notifications-channels.test.jstests/notifications-incidents-485.test.jstests/notifications-telegram-body-failure-485.test.js
Comment on lines
+3615
to
+3648
| const notificationOutcome = status === "PASS" ? "PASS" : retryDecision?.action === "exhausted" ? "EXHAUSTED" : "FAIL"; | ||
| const notificationRunDir = join(runsDir(projectRoot), manifest.run_id); | ||
| let notificationAction: string = "notify"; | ||
| try { | ||
| notificationAction = await applyNotificationIntent(notificationRunDir, incidentKeyForTask(task.id), notificationOutcome); | ||
| } catch (err) { | ||
| console.error("Failed to evaluate notification incident state (defaulting to notify):", err); | ||
| } | ||
| if (notificationAction === "update") { | ||
| await appendEvent(projectRoot, manifest.run_id, { | ||
| type: "notification_coalesced", | ||
| task_id: task.id, | ||
| outcome: notificationOutcome, | ||
| reason: "an external notification for this task's current failure streak was already sent", | ||
| }); | ||
| } else if (hasConfiguredChannels({ projectRoot })) { | ||
| try { | ||
| const payload = formatSlackStageMessage(manifest.run_id, task.id, status, { | ||
| message: status === "PASS" | ||
| ? `Task validated and advance targets committed. Summary: "${builderContract?.summary || "No summary recorded"}"` | ||
| : `Harness validation check failed for ${task.id}. Rerouting task to Builder Sandbox for corrections.`, | ||
| attempt: builderContract?.attempt || "1", | ||
| }); | ||
| await notifyAll(payload, { projectRoot }); | ||
|
|
||
| // Dispatch rich task execution report | ||
| const runDir = join(runsDir(projectRoot), manifest.run_id); | ||
| const report = await buildRunReport(runDir); | ||
| const messageByAction: Record<string, string> = { | ||
| open: `Harness validation check failed for ${task.id}. Rerouting task to Builder Sandbox for corrections.`, | ||
| escalate: `Task ${task.id} exhausted its retry budget and is now BLOCKED pending human override.`, | ||
| resolve: `Task ${task.id} passed after a prior failure. Incident closed.`, | ||
| resolve_from_escalation: `Task ${task.id} passed after an exhausted-retry override. Incident closed.`, | ||
| notify: `Task validated and advance targets committed. Summary: "${builderContract?.summary || "No summary recorded"}"`, | ||
| }; | ||
| const banner = messageByAction[notificationAction] ?? messageByAction.notify; | ||
| const displayStatus = notificationOutcome === "PASS" ? "PASS" : notificationOutcome === "EXHAUSTED" ? "BLOCKED" : "FAIL"; | ||
| const report = await buildRunReport(notificationRunDir); | ||
| const trace = report.tasks[task.id]; | ||
| if (trace) { | ||
| const reportPayload = formatSlackTaskReportMessage(manifest.run_id, task.id, trace); | ||
| await notifyAll(reportPayload, { projectRoot }); | ||
| } | ||
| const payload = trace | ||
| ? formatSlackTaskReportMessage(manifest.run_id, task.id, trace, { banner }) | ||
| : formatSlackStageMessage(manifest.run_id, task.id, displayStatus, { | ||
| message: banner, | ||
| attempt: builderContract?.attempt || "1", | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Error-fallback banner contradicts the real validation outcome.
If applyNotificationIntent throws (line 3619-3621), notificationAction stays at its default "notify". The banner lookup (messageByAction[notificationAction] ?? messageByAction.notify, line 3639) then always resolves to the "notify" message — "Task validated and advance targets committed..." — even when the real notificationOutcome is FAIL or EXHAUSTED. Meanwhile displayStatus (line 3640) correctly reflects the real outcome. The dispatched Slack message would show a success banner next to a FAIL/BLOCKED status field.
Derive the fallback banner from notificationOutcome, not from the unconditional "notify" default.
🐛 Proposed fix
+ const outcomeFallbackAction: Record<string, string> = { PASS: "notify", FAIL: "open", EXHAUSTED: "escalate" };
const messageByAction: Record<string, string> = {
open: `Harness validation check failed for ${task.id}. Rerouting task to Builder Sandbox for corrections.`,
escalate: `Task ${task.id} exhausted its retry budget and is now BLOCKED pending human override.`,
resolve: `Task ${task.id} passed after a prior failure. Incident closed.`,
resolve_from_escalation: `Task ${task.id} passed after an exhausted-retry override. Incident closed.`,
notify: `Task validated and advance targets committed. Summary: "${builderContract?.summary || "No summary recorded"}"`,
};
- const banner = messageByAction[notificationAction] ?? messageByAction.notify;
+ const banner = messageByAction[notificationAction]
+ ?? messageByAction[outcomeFallbackAction[notificationOutcome]]
+ ?? messageByAction.notify;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const notificationOutcome = status === "PASS" ? "PASS" : retryDecision?.action === "exhausted" ? "EXHAUSTED" : "FAIL"; | |
| const notificationRunDir = join(runsDir(projectRoot), manifest.run_id); | |
| let notificationAction: string = "notify"; | |
| try { | |
| notificationAction = await applyNotificationIntent(notificationRunDir, incidentKeyForTask(task.id), notificationOutcome); | |
| } catch (err) { | |
| console.error("Failed to evaluate notification incident state (defaulting to notify):", err); | |
| } | |
| if (notificationAction === "update") { | |
| await appendEvent(projectRoot, manifest.run_id, { | |
| type: "notification_coalesced", | |
| task_id: task.id, | |
| outcome: notificationOutcome, | |
| reason: "an external notification for this task's current failure streak was already sent", | |
| }); | |
| } else if (hasConfiguredChannels({ projectRoot })) { | |
| try { | |
| const payload = formatSlackStageMessage(manifest.run_id, task.id, status, { | |
| message: status === "PASS" | |
| ? `Task validated and advance targets committed. Summary: "${builderContract?.summary || "No summary recorded"}"` | |
| : `Harness validation check failed for ${task.id}. Rerouting task to Builder Sandbox for corrections.`, | |
| attempt: builderContract?.attempt || "1", | |
| }); | |
| await notifyAll(payload, { projectRoot }); | |
| // Dispatch rich task execution report | |
| const runDir = join(runsDir(projectRoot), manifest.run_id); | |
| const report = await buildRunReport(runDir); | |
| const messageByAction: Record<string, string> = { | |
| open: `Harness validation check failed for ${task.id}. Rerouting task to Builder Sandbox for corrections.`, | |
| escalate: `Task ${task.id} exhausted its retry budget and is now BLOCKED pending human override.`, | |
| resolve: `Task ${task.id} passed after a prior failure. Incident closed.`, | |
| resolve_from_escalation: `Task ${task.id} passed after an exhausted-retry override. Incident closed.`, | |
| notify: `Task validated and advance targets committed. Summary: "${builderContract?.summary || "No summary recorded"}"`, | |
| }; | |
| const banner = messageByAction[notificationAction] ?? messageByAction.notify; | |
| const displayStatus = notificationOutcome === "PASS" ? "PASS" : notificationOutcome === "EXHAUSTED" ? "BLOCKED" : "FAIL"; | |
| const report = await buildRunReport(notificationRunDir); | |
| const trace = report.tasks[task.id]; | |
| if (trace) { | |
| const reportPayload = formatSlackTaskReportMessage(manifest.run_id, task.id, trace); | |
| await notifyAll(reportPayload, { projectRoot }); | |
| } | |
| const payload = trace | |
| ? formatSlackTaskReportMessage(manifest.run_id, task.id, trace, { banner }) | |
| : formatSlackStageMessage(manifest.run_id, task.id, displayStatus, { | |
| message: banner, | |
| attempt: builderContract?.attempt || "1", | |
| }); | |
| const notificationOutcome = status === "PASS" ? "PASS" : retryDecision?.action === "exhausted" ? "EXHAUSTED" : "FAIL"; | |
| const notificationRunDir = join(runsDir(projectRoot), manifest.run_id); | |
| let notificationAction: string = "notify"; | |
| try { | |
| notificationAction = await applyNotificationIntent(notificationRunDir, incidentKeyForTask(task.id), notificationOutcome); | |
| } catch (err) { | |
| console.error("Failed to evaluate notification incident state (defaulting to notify):", err); | |
| } | |
| if (notificationAction === "update") { | |
| await appendEvent(projectRoot, manifest.run_id, { | |
| type: "notification_coalesced", | |
| task_id: task.id, | |
| outcome: notificationOutcome, | |
| reason: "an external notification for this task's current failure streak was already sent", | |
| }); | |
| } else if (hasConfiguredChannels({ projectRoot })) { | |
| try { | |
| const outcomeFallbackAction: Record<string, string> = { PASS: "notify", FAIL: "open", EXHAUSTED: "escalate" }; | |
| const messageByAction: Record<string, string> = { | |
| open: `Harness validation check failed for ${task.id}. Rerouting task to Builder Sandbox for corrections.`, | |
| escalate: `Task ${task.id} exhausted its retry budget and is now BLOCKED pending human override.`, | |
| resolve: `Task ${task.id} passed after a prior failure. Incident closed.`, | |
| resolve_from_escalation: `Task ${task.id} passed after an exhausted-retry override. Incident closed.`, | |
| notify: `Task validated and advance targets committed. Summary: "${builderContract?.summary || "No summary recorded"}"`, | |
| }; | |
| const banner = messageByAction[notificationAction] | |
| ?? messageByAction[outcomeFallbackAction[notificationOutcome]] | |
| ?? messageByAction.notify; | |
| const displayStatus = notificationOutcome === "PASS" ? "PASS" : notificationOutcome === "EXHAUSTED" ? "BLOCKED" : "FAIL"; | |
| const report = await buildRunReport(notificationRunDir); | |
| const trace = report.tasks[task.id]; | |
| const payload = trace | |
| ? formatSlackTaskReportMessage(manifest.run_id, task.id, trace, { banner }) | |
| : formatSlackStageMessage(manifest.run_id, task.id, displayStatus, { | |
| message: banner, | |
| attempt: builderContract?.attempt || "1", | |
| }); |
🤖 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 `@src/integrations/pi/rstack-sdlc.ts` around lines 3615 - 3648, Update the
notification fallback in the message construction around notificationAction and
messageByAction so an applyNotificationIntent failure derives the banner from
notificationOutcome rather than defaulting to the success-oriented "notify"
message. Preserve action-specific banners when notification intent evaluation
succeeds, while ensuring FAIL and EXHAUSTED outcomes use outcome-appropriate
messaging consistent with displayStatus.
Comment on lines
+3617
to
+3622
| let notificationAction: string = "notify"; | ||
| try { | ||
| notificationAction = await applyNotificationIntent(notificationRunDir, incidentKeyForTask(task.id), notificationOutcome); | ||
| } catch (err) { | ||
| console.error("Failed to evaluate notification incident state (defaulting to notify):", err); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect buildRunReport's readJson helper for error-swallowing behavior.
ast-grep run --pattern 'function readJson($$$) { $$$ }' --lang javascript src/observability/collectors/reporter.js
rg -n -B2 -A10 'export (async )?function readJson' src/observabilityRepository: richard-devbot/SDLC-rstack
Length of output: 3177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant functions and call sites without executing repository code.
printf 'Files containing buildRunReport/readWriteJson/readJson definitions/usages:\n'
rg -n "buildRunReport|readWriteJson|function readJson|readJsonTracked|metrics_write_failed|notificationAction|notifyAll|appendEvent" src/integrations/pi/rstack-sdlc.ts src/observability || true
printf '\n--- rstack-sdlc.ts relevant slice 3340-3420 ---\n'
sed -n '3340,3420p' src/integrations/pi/rstack-sdlc.ts | cat -n -v
printf '\n--- rstack-sdlc.ts relevant slice 3590-3675 ---\n'
sed -n '3590,3675p' src/integrations/pi/rstack-sdlc.ts | cat -n -v
printf '\n--- reporter.js definitions ---\n'
sed -n '1,120p' src/observability/collectors/reporter.js | cat -n -v
printf '\n--- rstack-sdlc.ts relevant definitions with ast-grep ---\n'
ast-grep run --lang javascript --pattern 'export async function buildRunReport($_) { $$$ }' src/integrations/pi/rstack-sdlc.ts || true
ast-grep run --lang javascript --pattern 'let $REPORT = buildRunReport($_);' src/integrations/pi/rstack-sdlc.ts || trueRepository: richard-devbot/SDLC-rstack
Length of output: 33807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/integrations/pi/rstack-sdlc.ts appendEvent implementation ---\n'
sed -n '810,875p' src/integrations/pi/rstack-sdlc.ts | cat -n -v
printf '\n--- src/observability/collectors/reporter.js buildRunReport tail ---\n'
sed -n '73,215p' src/observability/collectors/reporter.js | cat -n -v
printf '\n--- notifications/incidents.ts key symbols ---\n'
sed -n '1,240p' src/notifications/incidents.ts | cat -n -v 2>/dev/null || true
printf '\n--- deterministic probe of buildRunReport error sources from source text ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('src/observability/collectors/reporter.js')
for i,line in enumerate(p.read_text().splitlines(),1):
if 73 <= i <= 215:
if 'throw' in line or 'readFile' in line or 'readJson' in line or 'readJsonl' not in line and 'buildRunReport' in line:
print(f'{i}: {line}')
PYRepository: richard-devbot/SDLC-rstack
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/integrations/pi/rstack-sdlc.ts appendEvent implementation ---'
sed -n '810,875p' src/integrations/pi/rstack-sdlc.ts | cat -n -v
printf '\n%s\n' '--- src/observability/collectors/reporter.js buildRunReport tail ---'
sed -n '73,215p' src/observability/collectors/reporter.js | cat -n -v
printf '\n%s\n' '--- notifications/incidents key symbols ---'
sed -n '1,240p' src/notifications/incidents.ts | cat -n -v 2>/dev/null || true
printf '\n%s\n' '--- deterministic probe of buildRunReport error sources from source text ---'
python3 - <<'PY'
from pathlib import Path
p = Path('src/observability/collectors/reporter.js')
for i,line in enumerate(p.read_text().splitlines(),1):
if 73 <= i <= 215:
if 'throw' in line or 'readFile' in line or 'readJson' in line or 'readJsonl' not in line and 'buildRunReport' in line:
print(f'{i}: {line}')
PYRepository: richard-devbot/SDLC-rstack
Length of output: 11661
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- locate notifications/incidents files ---'
fd -i 'incidents' src || true
printf '\n%s\n' '--- notification related files in src/notifications ---'
fd . src/notifications -t f -x sh -c 'echo "--- $1 ---"; sed -n "1,260p" "$1" | cat -n -v' sh {}
printf '\n%s\n' '--- notification dispatch try block in rstack-sdlc.ts ---'
sed -n '3636,3676p' src/integrations/pi/rstack-sdlc.ts | cat -n -vRepository: richard-devbot/SDLC-rstack
Length of output: 50382
Do not leave an opened notification incident without a compensating dispatch event.
applyNotificationIntent commits open before buildRunReport, notifyAll, and notification_dispatched run. If buildRunReport fails, the code falls back to logging, but the later retry can return update and avoid notifying the already-open incident. Emit a pinned event for this dispatch failure, or separate the report build fallback and dispatch failure recording so the drop cannot be hidden behind an already-open incident.
🤖 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 `@src/integrations/pi/rstack-sdlc.ts` around lines 3617 - 3622, The
notification flow around applyNotificationIntent must record a compensating
pinned dispatch-failure event whenever buildRunReport, notifyAll, or
notification_dispatched fails after an incident is opened. Separate
report-building fallback from dispatch failure handling, and ensure retries
cannot silently return update for an already-open incident without recording the
failed dispatch.
Comment on lines
+25
to
+27
| * State is a tiny, pure, injectable-io JSON file per run: | ||
| * .rstack/runs/<runId>/notification-incidents.json | ||
| * shape: { [incidentKey]: { status: 'open'|'escalated', opened_at, attempts } } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add a schema_version to the persisted incident record.
The documented shape ({ status, opened_at, attempts }, lines 25-27) and every state-creation site in evaluateNotificationIntent omit a version marker. A future change to this shape (e.g., adding a field, renaming attempts) will have no way to distinguish an old on-disk record from a new one when the file is read back by loadIncidentState.
Add a schema_version field to the persisted record now, while the shape is still new, to avoid an unversioned migration problem later.
As per coding guidelines, "Preserve authoritative, atomic, schema-versioned, inspectable pipeline state; never silently degrade state."
Also applies to: 76-96
🤖 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 `@src/notifications/incidents.js` around lines 25 - 27, Add a schema_version
field to the documented incident record and every new-record construction path
in evaluateNotificationIntent, using one consistent current version value.
Update loadIncidentState to preserve and validate this marker when reading
persisted state, rejecting or explicitly handling records with missing or
unsupported versions rather than silently accepting them.
Source: Coding guidelines
Comment on lines
+46
to
+58
| export function loadIncidentState(runDir) { | ||
| const path = incidentsPath(runDir); | ||
| if (!existsSync(path)) return {}; | ||
| try { | ||
| const parsed = JSON.parse(readFileSync(path, 'utf8')); | ||
| return parsed && typeof parsed === 'object' && !Array.isArray(parsed) ? parsed : {}; | ||
| } catch { | ||
| // A corrupt incidents file must never block validation — degrade to no | ||
| // known incidents (worst case: one extra notification is sent, never a | ||
| // dropped one). | ||
| return {}; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Corrupt-incidents-file handling is unobservable and untested. loadIncidentState's catch branch degrades a malformed notification-incidents.json to {} with no log line, and the test suite's own test named for this case never actually writes a malformed file, so the branch runs unverified in production.
src/notifications/incidents.js#L46-L58: add aconsole.errorin thecatchblock before returning{}, matching the pattern this PR already applies to malformednotifications.json/policy.json.tests/notifications-incidents-485.test.js#L100-L107: extend the test to actually write an invalid JSON payload tonotification-incidents.jsonand assertloadIncidentStatereturns{}, so the catch branch (and the new log line) is exercised.
📍 Affects 2 files
src/notifications/incidents.js#L46-L58(this comment)tests/notifications-incidents-485.test.js#L100-L107
🤖 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 `@src/notifications/incidents.js` around lines 46 - 58, Update
src/notifications/incidents.js lines 46-58 in loadIncidentState to log the
malformed incidents-file error with console.error before returning {}. Update
tests/notifications-incidents-485.test.js lines 100-107 to write invalid JSON to
notification-incidents.json and assert loadIncidentState returns {}, exercising
the catch branch and log behavior.
Source: Coding guidelines
Comment on lines
52
to
61
| if (parsed?.channels != null && typeof parsed.channels !== 'object') { | ||
| // #485 (audit finding, verbatim): "Malformed or missing routing | ||
| // configuration can fail silently." A well-formed-JSON-but-wrong-shape | ||
| // `channels` (a string, array, etc.) used to degrade to {} with no | ||
| // signal at all — every configured channel silently vanishes. Loud | ||
| // (but still non-blocking, per this module's fire-and-forget layer | ||
| // rule) at the point of use, not only at doctor/dashboard time. | ||
| console.error(`[rstack notifications] ${path}: "channels" must be an object keyed by channel name — got ${typeof parsed.channels}; every configured channel is being dropped. Run \`rstack-agents doctor\` for the full diagnosis.`); | ||
| } | ||
| return parsed?.channels && typeof parsed.channels === 'object' ? parsed.channels : {}; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject array and non-object notification configuration.
Line 52 accepts arrays because typeof [] === "object". Line 61 then returns the array. resolveChannels() spreads its numeric keys and filters them because no sender has a numeric name. A top-level array, string, or null also returns {} without a diagnostic. These malformed configurations silently disable all file-configured channels.
Require a non-array object for both the top-level document and channels. Add regression tests for top-level and channels arrays.
As per coding guidelines, “Unknown, corrupt, missing, or unreadable state must produce an honest unknown, warning, or failure rather than a false green, false zero, or silently skipped gate.”
Proposed fix
function fileConfig(projectRoot) {
if (!projectRoot) return {};
const path = join(projectRoot, '.rstack', 'notifications.json');
if (!existsSync(path)) return {};
try {
const parsed = JSON.parse(readFileSync(path, 'utf8'));
- if (parsed?.channels != null && typeof parsed.channels !== 'object') {
+ const isObjectMap = (value) =>
+ value !== null && typeof value === 'object' && !Array.isArray(value);
+ if (!isObjectMap(parsed)) {
+ console.error(`[rstack notifications] ${path} must be a JSON object at the top level; every configured channel is being dropped.`);
+ return {};
+ }
+ if (parsed.channels != null && !isObjectMap(parsed.channels)) {
console.error(`[rstack notifications] ${path}: "channels" must be an object keyed by channel name ...`);
}
- return parsed?.channels && typeof parsed.channels === 'object' ? parsed.channels : {};
+ return isObjectMap(parsed.channels) ? parsed.channels : {};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (parsed?.channels != null && typeof parsed.channels !== 'object') { | |
| // #485 (audit finding, verbatim): "Malformed or missing routing | |
| // configuration can fail silently." A well-formed-JSON-but-wrong-shape | |
| // `channels` (a string, array, etc.) used to degrade to {} with no | |
| // signal at all — every configured channel silently vanishes. Loud | |
| // (but still non-blocking, per this module's fire-and-forget layer | |
| // rule) at the point of use, not only at doctor/dashboard time. | |
| console.error(`[rstack notifications] ${path}: "channels" must be an object keyed by channel name — got ${typeof parsed.channels}; every configured channel is being dropped. Run \`rstack-agents doctor\` for the full diagnosis.`); | |
| } | |
| return parsed?.channels && typeof parsed.channels === 'object' ? parsed.channels : {}; | |
| const isObjectMap = (value) => | |
| value !== null && typeof value === 'object' && !Array.isArray(value); | |
| if (!isObjectMap(parsed)) { | |
| console.error(`[rstack notifications] ${path} must be a JSON object at the top level; every configured channel is being dropped.`); | |
| return {}; | |
| } | |
| if (parsed.channels != null && !isObjectMap(parsed.channels)) { | |
| console.error(`[rstack notifications] ${path}: "channels" must be an object keyed by channel name ...`); | |
| } | |
| return isObjectMap(parsed.channels) ? parsed.channels : {}; |
🤖 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 `@src/notifications/router.js` around lines 52 - 61, Update the configuration
validation and return logic around the visible channels parsing flow to require
a non-null, non-array object for both the top-level parsed document and
parsed.channels; arrays must be rejected rather than returned or spread by
resolveChannels(). Emit the existing non-blocking diagnostic for malformed
top-level and channels configurations, while preserving {} as the fallback. Add
regression tests covering top-level arrays and channels arrays.
Source: Coding guidelines
Comment on lines
206
to
+209
| const bad = await sendEmail({ connection_string: 'garbage', sender: 'a@b.co' }, PAYLOAD, META); | ||
| assert.match(bad, /failed/, 'malformed connection string logged, never thrown'); | ||
| assert.ok(!bad.includes('garbage') || true, 'status string is bounded'); | ||
| assert.equal(bad.ok, false, 'a malformed connection string is a real failure'); | ||
| assert.match(bad.message, /failed/, 'malformed connection string logged, never thrown'); | ||
| assert.ok(!bad.message.includes('garbage') || true, 'status message is bounded'); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the bounded-message assertion effective.
Line 209 always passes because it ends with || true. The test does not verify the 120-character error-message bound in src/notifications/channels/email.js Line 143.
Assert the maximum message length directly.
As per coding guidelines, “Use TDD: tests must demonstrate failure before the implementation fix, and tests must land with the change they verify.”
Proposed fix
- assert.ok(!bad.message.includes('garbage') || true, 'status message is bounded');
+ assert.ok(
+ bad.message.length <= 'email: failed ()'.length + 120,
+ 'status message is bounded',
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bad = await sendEmail({ connection_string: 'garbage', sender: 'a@b.co' }, PAYLOAD, META); | |
| assert.match(bad, /failed/, 'malformed connection string logged, never thrown'); | |
| assert.ok(!bad.includes('garbage') || true, 'status string is bounded'); | |
| assert.equal(bad.ok, false, 'a malformed connection string is a real failure'); | |
| assert.match(bad.message, /failed/, 'malformed connection string logged, never thrown'); | |
| assert.ok(!bad.message.includes('garbage') || true, 'status message is bounded'); | |
| const bad = await sendEmail({ connection_string: 'garbage', sender: 'a@b.co' }, PAYLOAD, META); | |
| assert.equal(bad.ok, false, 'a malformed connection string is a real failure'); | |
| assert.match(bad.message, /failed/, 'malformed connection string logged, never thrown'); | |
| assert.ok( | |
| bad.message.length <= 'email: failed ()'.length + 120, | |
| 'status message is bounded', | |
| ); |
🤖 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 `@tests/email-approvals-353.test.js` around lines 206 - 209, Update the
malformed-connection assertion in the sendEmail test to directly verify that
bad.message does not exceed the 120-character limit enforced by the email
implementation. Remove the unconditional “|| true” expression while preserving
the existing failure-status and non-throwing assertions.
Source: Coding guidelines
…rray misdetection, write churn Three real findings from Qodo's review of PR #522: - Critical correctness bug: applyNotificationIntent() ran BEFORE the hasConfiguredChannels() check, so a run with no channels configured would still open/advance an incident with nobody ever notified. Once a channel was configured mid-run, the next FAIL would read as an "update" on that phantom pre-existing incident and suppress the very first notification anyone would have received. Fixed by gating the incident engine on hasConfiguredChannels first — no incident state is created while nothing can be delivered. - router.js's malformed-channels check used `typeof x !== 'object'`, which doesn't exclude arrays (arrays are typeof 'object' in JS) — an array-valued `channels` field bypassed the new loud-on-malformed log and was spread as numeric-string-keyed junk, which could make hasConfiguredChannels() report true while notifyAll() found no matching sender for any of them. - applyNotificationIntent() unconditionally rewrote notification- incidents.json even on a true no-op (a routine first-attempt PASS with no open incident) — now skips the write when nothing actually changed, so a run that never fails never gets an incidents file at all. Also fixed a real bug in this PR's OWN test cleanup, caught while mutation-testing the incident-suppression fix: `process.env.X = undefined` sets the literal string "undefined" (still truthy), not delete — several tests were leaking a stale RSTACK_SLACK_WEBHOOK into later tests in the same file. Replaced with `delete process.env.X`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Closes the pieces of #485 that are safely shippable this slice — the full transactional-outbox/coalescing-policy/retry-backoff-dead-letter design in the issue is a separate, much larger subsystem (comparable in size to the #481 attempt-ledger build), documented as deferred in
docs/HARNESS.md.notifyAll()only treated a THROWN sender error asok:false.sendEmail's deliberate "never throws" contract returned bare status strings on every failure path — all recorded as delivered.sendEmailnow returns{ ok, message };notifyAllinterprets it directly. Found the same bug class one level lower: Telegram's Bot API can return HTTP 200 with a body-level failure thatpostJson's status-code-only check missed —sendTelegramnow parses the body and throws.sdlc_validatefired 2 messages per call regardless of PASS/FAIL, plus a 3rd on exhaustion, with zero memory across retries. Newsrc/notifications/incidents.js: one incident per task's current unbroken failure streak — FAIL on an open incident suppresses the external notification (internally recorded vianotification_coalesced), exhaustion escalates once, a later PASS resolves and clears it. Every dispatch attempt is recorded via a newnotification_dispatchedevent (per-channel{channel, ok}), directly serving the "delivery status is accurate and queryable" acceptance criterion.notifications.json/policy.jsonused to degrade to{}/nullwith zero signal. Both nowconsole.errorat the point of the actual runtime degrade.Deferred (documented in
docs/HARNESS.md): the full transactional notification outbox tied to #481's ledger, durable retry/backoff, a dead-letter queue, rate bounds/quiet hours, and a Business Hub incident UI. None of these exist anywhere in this codebase today.Test plan
npm run typecheck— cleannpm test— 1864/1864 passing (4 consecutive full runs, confirming a pre-existing dashboard-server test flake unrelated to this change — different tests failed each of two runs, both passed cleanly in isolation, then two more full runs passed clean)npm run lint— 0 errors (4 pre-existing unrelated warnings)node scripts/security-audit.mjs— clean (1 pre-existing tolerated bundled advisory)npm run validate— 196/196 agents passgit diff --check— clean, nopackage-lock.jsondrifttests/notifications-incidents-485.test.js,tests/notifications-telegram-body-failure-485.test.js,tests/extension-notification-coalescing-485.test.js, plus updated assertions intests/email-approvals-353.test.jsandtests/notifications-channels.test.js🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation