feat: add decision readiness gate#101
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
📝 WalkthroughWalkthroughAdds a run-level Decision Queue and a Definition-of-Ready readiness gate: decision persistence and CRUD, DoR stage/profile checks with report persistence, CLI subcommands and Pi tools, dashboard state/UI, documentation and package updates, plus end-to-end tests and validation tweaks. ChangesDecision Queue and Definition-of-Ready Feature
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![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: 5
🤖 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/core/harness/decisions.js`:
- Around line 68-85: The read-modify-write of decisions.json in
writeDecisions/addDecision (and decide) is racy; implement a per-run atomic
update (either a filesystem-backed per-run lock or a compare-and-swap retry
loop) so ID assignment and persistence are serialized. Change addDecision to
acquire the run lock (or loop: read file, compute next DEC-### from the latest
contents, create normalized entry, attempt atomic replace by writing to a temp
file and renaming or by checking updated_at and retrying on mismatch) and
release the lock (or break on success); do the same for decide and any code path
that calls writeDecisions so you always recompute the next number after
acquiring the lock/confirming no concurrent update, retry a bounded number of
times, and surface an error if retries are exhausted.
In `@src/core/harness/readiness.js`:
- Around line 22-29: The current stageIndex maps unknown IDs to
STAGE_ORDER.length which lets mistyped or new required_before_stage values
bypass readiness; update stageIndex(stageId) to return -1 for unknown IDs
(instead of STAGE_ORDER.length) and change isRequiredBefore(decision,
targetStage) to explicitly reject unknown stage ids by throwing an Error when
stageIndex(decision.required_before_stage) === -1 or when targetStage is
provided and stageIndex(targetStage) === -1; this enforces failing fast for
unknown stages (or alternatively treat unknown required_before_stage as always
required by returning true) and references the existing functions stageIndex and
isRequiredBefore and the STAGE_ORDER constant.
In `@src/integrations/pi/rstack-sdlc.ts`:
- Around line 1480-1491: The DoR gating should follow assertReadyForStage's
contract: only FAIL (ok===false) blocks, and WARN may be informational when
there are no pending decisions; modify the warning logic in rstack-sdlc.ts so
that appendEvent(..., { type: "dor_gate_warning", ... }) and any WARN-specific
handling only runs when readiness.report.status === "WARN" AND
readiness.report.pending_required?.length > 0; keep the existing FAIL branch
that appends dor_gate_blocked and returns the blocked response (using
readiness.report.pending_required as before). Ensure you reference
assertReadyForStage, readiness.report, pending_required, appendEvent,
dor_gate_warning and dor_gate_blocked when making the change.
In `@src/observability/dashboard/state/decisions.js`:
- Around line 21-29: In the catch block where entries.push is used, stop
treating read/check failures as a PASS: change the catch to capture the
exception (catch (err)) and set the fallback readiness object to indicate
non-success (e.g., readiness.status = 'WARN' or 'UNKNOWN') and add an error
marker and message (e.g., readiness.error = true and readiness.errorMessage =
String(err) or err.message) so operators can distinguish unavailable runs; keep
or adjust readiness.score appropriately (e.g., null or 0) and ensure you
reference the readiness object created inside the entries.push call and the
caught error variable (err).
In `@src/observability/dashboard/ui/client.js`:
- Around line 268-272: The copy currently filters copy.decisions.runs by runIds
but leaves copy.decisions.totals unchanged, causing totals to not reflect the
scoped rows; after creating copy.decisions.runs (the filtered array), recompute
copy.decisions.totals from that filtered array (e.g., recalc counts/metrics used
in totals based on the items in copy.decisions.runs) so totals align with the
displayed rows; update the block that handles s.decisions / copy.decisions to
replace the existing totals with the derived totals computed from the filtered
runs.
🪄 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: ce5eba1f-ccae-495d-991a-3a7ac26f5c3a
📒 Files selected for processing (17)
README.mdbin/rstack-agents.jsdocs/mintlify/docs.jsondocs/mintlify/reference/decision-readiness.mdxpackage.jsonsrc/core/harness/decisions.jssrc/core/harness/readiness.jssrc/index.jssrc/integrations/pi/rstack-sdlc.tssrc/observability/dashboard/state/decisions.jssrc/observability/dashboard/state/index.jssrc/observability/dashboard/ui/client.jssrc/observability/dashboard/ui/pages/index.jstests/decisions-readiness.test.jstests/validate-extension.test.jstests/validate-package-assets.test.jstests/validate-references.test.js
- Serialize Decision Queue writes with a per-run filesystem lock\n- Fail closed on unknown readiness stages\n- Surface readiness load failures as warnings\n- Recompute scoped dashboard decision totals\n\nAddresses CodeRabbit comments on #101
…test stage Review follow-ups on #101: - withDecisionLock now steals locks orphaned by a crashed holder (mtime older than 30s) instead of timing out forever, and the EEXIST retry is scoped to the lock acquire so errors from the locked operation are not swallowed or re-executed. - resolveRunId rejects run ids containing path separators or traversal so --run-id cannot write state outside .rstack. - sdlc_build_next gates on the latest stage of a bundled task via latestStageId() instead of the first, so decisions required before a mid-bundle stage actually block. - Tests for all three; merged main (CRLF frontmatter fixes from #100). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
Review + hardening passReviewed the full diff. All four earlier CodeRabbit findings genuinely landed (lock + atomic rename, fail-closed unknown stages, WARN fallback on read errors, scoped totals recompute). Windows path handling, XSS escaping in the new dashboard rendering, and test isolation all check out. Pushed two follow-up commits addressing what the review surfaced:
Also merged 🤖 Generated with Claude Code |
Contributor
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)
src/core/harness/readiness.js (1)
15-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap
business-flextowarn, notapproval.Line 19 currently reports
approvalfor every non-enterprise-webappprofile, which includesbusiness-flex. That conflicts with the PR objective thatbusiness-flexshould default to warn, and it leaks the wrong mode intodor-report.json, CLI/Pi responses, and the new test expectations.Proposed fix
export function readinessModeForProfile(profile) { const name = typeof profile === 'string' ? profile : profile?.profile; if (name === 'enterprise-webapp') return 'blocking'; if (name === 'lean-mvp') return 'warn'; - return 'approval'; + return 'warn'; }🤖 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/core/harness/readiness.js` around lines 15 - 20, The readinessModeForProfile function currently defaults all non-enterprise-webapp profiles to 'approval', which misclassifies 'business-flex'; update readinessModeForProfile to explicitly return 'warn' for the 'business-flex' profile (e.g., add an if (name === 'business-flex') return 'warn' before the final return) so that 'business-flex' maps to 'warn' while keeping enterprise-webapp => 'blocking', lean-mvp => 'warn', and all others => 'approval'.
🤖 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 `@src/core/harness/readiness.js`:
- Around line 15-20: The readinessModeForProfile function currently defaults all
non-enterprise-webapp profiles to 'approval', which misclassifies
'business-flex'; update readinessModeForProfile to explicitly return 'warn' for
the 'business-flex' profile (e.g., add an if (name === 'business-flex') return
'warn' before the final return) so that 'business-flex' maps to 'warn' while
keeping enterprise-webapp => 'blocking', lean-mvp => 'warn', and all others =>
'approval'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a183296-a378-4c17-a3e4-3d9a5775be67
📒 Files selected for processing (6)
README.mdbin/rstack-agents.jssrc/core/harness/decisions.jssrc/core/harness/readiness.jssrc/integrations/pi/rstack-sdlc.tstests/decisions-readiness.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- bin/rstack-agents.js
- src/integrations/pi/rstack-sdlc.ts
- src/core/harness/decisions.js
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
2 participants
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
decisions.json) with pending/resolved/waived decisions and stale-decision summaries.dor-report.json,readiness.json) and profile-aware gate behavior: business-flex warns, enterprise-webapp blocks.Test Plan
npx tsx --test tests/validate-extension.test.js tests/validate-references.test.js tests/decisions-readiness.test.jsnpm run lintnpm testnpm run validategit diff --checknpm pack --dry-run --jsonverifieddecision-readiness.mdxanddocs.jsonshipCloses #70
Summary by CodeRabbit
New Features
Documentation
Tests