fix: scope ambient events + destructive approvals to the session run; init --fresh (#98, #99)#100
Open
richardsongunde wants to merge 3 commits into
Open
Conversation
parseFrontmatter (CLI + tests) split on \n and matched lines with a line-anchored regex; '.' does not match \r and '$' does not precede a trailing \r, so on CRLF checkouts every field line failed to parse and all 196 agents reported missing name/description. Normalize CRLF/CR before parsing. Add .gitattributes to keep text assets LF on checkout. Fixes: validate (196/196 pass), test 'valid frontmatter' + 'valid unique Pi skill names' now pass. Closes #1
…n; add init --fresh Fixes richard-devbot#98: tool_call/tool_result/session_shutdown hooks and destructiveApprovalExists resolved the target run via latestRun(), which returns whatever run directory sorts last — so a new session appended its events into a stale run from days earlier, and a destructive-action approval granted in an old run silently authorized destructive commands in a new session. Ambient hooks, approval checks, and model escalation now use a session-scoped run set by sdlc_start; with no session run they stay silent instead of polluting old runs. Fixes richard-devbot#99: init on an existing .rstack/ now reports how many prior runs it adopted, and a new --fresh flag archives runs, approvals, memory, registry, and config to .rstack/archive/<timestamp>/ (nothing deleted) before initializing clean. Documented in README. Tests: 3 new init subtests (prior-run reporting, --fresh archival, --fresh on clean project). npm test shows the same 7 pre-existing Windows-environment failures as unmodified main; lint and validate pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
📝 WalkthroughWalkthroughThe PR extends SDLC reliability through cross-platform line-ending normalization in validation, adds a ChangesSDLC Reliability and Init Workflow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 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 unit tests (beta)
Comment |
![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: 2
🧹 Nitpick comments (1)
tests/integrations-init.test.js (1)
176-201: ⚡ Quick winConsider expanding test coverage to validate archival of all ARCHIVABLE_STATE entries.
The test currently verifies archival of
runs,approvals.jsonl, andrstack.config.json, but ARCHIVABLE_STATE also includesmemory,registry, andbudget.json. While not critical for the current PR (the core archival mechanism is validated), adding assertions for these entries would strengthen confidence that the feature works comprehensively.🤖 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/integrations-init.test.js` around lines 176 - 201, The test "init --fresh archives prior state non-destructively and starts clean" currently checks runs, approvals.jsonl, and rstack.config.json but misses other ARCHIVABLE_STATE entries; update the test (the async function with tmpProject root and call to initFramework(root, 'custom', { profile: 'lean-mvp', fresh: true })) to create representative files for memory, registry, and budget.json under root/.rstack before init, then after init assert those files no longer exist in root and assert they exist under join(archiveRoot, stamp, ...) (use the same archiveRoot and stamp variables already in the test) to validate archival of each ARCHIVABLE_STATE entry.
🤖 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 `@README.md`:
- Around line 22-26: Update the README snippet to accurately list all items
moved during a fresh init by matching the implementation constant
ARCHIVABLE_STATE in src/integrations/init.js: include "runs, approvals,
registry, config, memory, budget.json" (or "memory and budget.json") in the
archival description so the docs reflect the actual archived state.
In `@src/commands/validate.js`:
- Around line 27-33: Both other frontmatter parsers must apply the same CRLF/CR
normalization as validate.js: in the parseFrontmatter function in
src/commands/list.js replace direct uses of content.indexOf and content.slice
with a normalized string (e.g., const normalized = content.replace(/\r\n?/g,
'\n')) and then use normalized.startsWith, normalized.indexOf('\n---', 3) and
normalized.slice(...) so fences with \r\n are detected; likewise in
src/integrations/pi/rstack-sdlc.ts normalize the raw input before calling
raw.indexOf("\n---", 3) (use a normalized variable and use
normalized.indexOf(...) and normalized.slice(...)) so all frontmatter searches
consistently operate on CRLF-normalized text.
---
Nitpick comments:
In `@tests/integrations-init.test.js`:
- Around line 176-201: The test "init --fresh archives prior state
non-destructively and starts clean" currently checks runs, approvals.jsonl, and
rstack.config.json but misses other ARCHIVABLE_STATE entries; update the test
(the async function with tmpProject root and call to initFramework(root,
'custom', { profile: 'lean-mvp', fresh: true })) to create representative files
for memory, registry, and budget.json under root/.rstack before init, then after
init assert those files no longer exist in root and assert they exist under
join(archiveRoot, stamp, ...) (use the same archiveRoot and stamp variables
already in the test) to validate archival of each ARCHIVABLE_STATE entry.
🪄 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: a21547e9-53b1-43d7-a226-5a2d3b09c07a
📒 Files selected for processing (9)
.gitattributesREADME.mdbin/rstack-agents.jssrc/commands/validate.jssrc/integrations/init.jssrc/integrations/pi/rstack-sdlc.tstests/integrations-init.test.jstests/validate-agents.test.jstests/validate-markdown-frontmatter.test.js
Comment on lines
+22
to
+26
| If `.rstack/` already exists, `init` adopts it and preserves all prior runs — it reports how many it found. To start clean instead, archive the old state (nothing is deleted): | ||
|
|
||
| ```bash | ||
| npx rstack-agents init --fresh # moves runs, approvals, registry, config to .rstack/archive/<timestamp>/ | ||
| ``` |
Contributor
There was a problem hiding this comment.
Documentation is incomplete.
Line 26 lists archived items as "runs, approvals, registry, config" but the implementation (ARCHIVABLE_STATE in src/integrations/init.js:49) also archives memory and budget.json. Update the comment to include all archived items for accuracy.
📝 Suggested fix
-npx rstack-agents init --fresh # moves runs, approvals, registry, config to .rstack/archive/<timestamp>/
+npx rstack-agents init --fresh # moves runs, approvals, memory, registry, config, budget to .rstack/archive/<timestamp>/🤖 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 `@README.md` around lines 22 - 26, Update the README snippet to accurately list
all items moved during a fresh init by matching the implementation constant
ARCHIVABLE_STATE in src/integrations/init.js: include "runs, approvals,
registry, config, memory, budget.json" (or "memory and budget.json") in the
archival description so the docs reflect the actual archived state.
Comment on lines
27
to
+33
| function parseFrontmatter(content) { | ||
| if (!content.startsWith('---')) return null; | ||
| const end = content.indexOf('\n---', 3); | ||
| // Normalize CRLF/CR so the line-anchored regex below matches on Windows checkouts. | ||
| const normalized = content.replace(/\r\n?/g, '\n'); | ||
| if (!normalized.startsWith('---')) return null; | ||
| const end = normalized.indexOf('\n---', 3); | ||
| if (end === -1) return null; | ||
| const block = content.slice(3, end); | ||
| const block = normalized.slice(3, end); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all parseFrontmatter and frontmatterBlock implementations
echo "=== Searching for frontmatter parsing functions ==="
rg -n -A 5 'function (parseFrontmatter|frontmatterBlock)' --type js --type ts
echo -e "\n=== Checking which implementations normalize CRLF/CR ==="
rg -n 'replace\(/\\r\\n\?/g' --type js --type ts
echo -e "\n=== Finding delimiter searches that may fail on CRLF ==="
rg -n "indexOf\(['\"]\\\\n---['\"]" --type js --type tsRepository: richard-devbot/SDLC-rstack
Length of output: 3394
Apply CRLF normalization consistently to all frontmatter parsers.
src/commands/list.js:parseFrontmatterdoes not normalize CRLF/CR (still usescontent.indexOf('\n---', 3)), unlikesrc/commands/validate.js.src/integrations/pi/rstack-sdlc.ts:parseFrontmatteralso lacks CRLF normalization and searches forraw.indexOf("\n---", 3)(plus anotherraw.indexOf("\n---", 3)around line ~758), which will fail when fences use\r\n.
🤖 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/commands/validate.js` around lines 27 - 33, Both other frontmatter
parsers must apply the same CRLF/CR normalization as validate.js: in the
parseFrontmatter function in src/commands/list.js replace direct uses of
content.indexOf and content.slice with a normalized string (e.g., const
normalized = content.replace(/\r\n?/g, '\n')) and then use
normalized.startsWith, normalized.indexOf('\n---', 3) and normalized.slice(...)
so fences with \r\n are detected; likewise in src/integrations/pi/rstack-sdlc.ts
normalize the raw input before calling raw.indexOf("\n---", 3) (use a normalized
variable and use normalized.indexOf(...) and normalized.slice(...)) so all
frontmatter searches consistently operate on CRLF-normalized text.
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
tool_call/tool_result/session_shutdownevents were appended to whatever run directory sorted last under.rstack/runs/(latestRun()), i.e. a stale run from a previous session. Worse,destructiveApprovalExists()read approvals from that same stale run, so adestructive-actionapproval granted days ago silently authorized destructive bash commands in a brand-new session.initon a pre-existing.rstack/now reports how many prior runs it adopted, and a new--freshflag archives prior state (runs, approvals, memory, registry, config, budget) to.rstack/archive/<timestamp>/— nothing is ever deleted — before initializing clean. README documents the adopt-don't-overwrite behavior.Changes
src/integrations/pi/rstack-sdlc.ts: new module-scopedsessionRunIdset bysdlc_start. Ambient hooks,destructiveApprovalExists(), and delegate model-escalation now usesessionRun()instead oflatestRun(). With no session-owned run they stay silent rather than polluting an old run. Explicitrun_idparameters on user-facing tools (sdlc_status,sdlc_rollback, …) are unchanged.src/integrations/init.js: prior-run count in the init report;--fresharchival (non-destructive,renameinto.rstack/archive/<ts>/).bin/rstack-agents.js:--freshCLI flag.tests/integrations-init.test.js: 3 new subtests (prior-run reporting, --fresh archival preserves everything, --fresh on a clean project).README.md: documents adoption behavior +--fresh.Design note
Session adoption happens only on
sdlc_start. A session that resumes an old run via explicitrun_idparams still writes its stage events (those use explicitappendEventcalls), but ambient hook logging stays off — accurate attribution was preferred over completeness. Follow-up could add an explicitsdlc_resume-style adoption.Validation
npx tsx --test tests/integrations-init.test.js— 18 pass, 0 fail (3 new)npm test— same 7 pre-existing Windows-environment failures as unmodifiedmain(ESMfile://dynamic-import in tests, operator-bridge spawn, memory path); zero new failuresnpm run lint— cleannpm run validate— all 196 agents passnode bin/rstack-agents.js init --helpshows--freshReal-world repro this fixes
On a workspace with a run from 2026-06-01 (manifest stuck
STARTED), a session on 2026-06-09 appended its bash tool calls and even its ownsdlc_startinvocation into the June 1 run'sevents.jsonlbefore the new run folder existed — polluting Run Analytics and the Business Hub timeline for both runs.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--freshflag toinitcommand to archive existing state and start with a clean configuration.Bug Fixes
Documentation
--freshflag usage.Tests