fix: improve handoff accuracy and add roadmap validation#25
Merged
Conversation
- Fix cmd_update_handoff() to populate Completed Work table, remove (none yet) placeholder, and auto-advance Next Phase Context - Add _detect_wave_for_package() to find wave labels from root ROADMAP - Add _advance_next_phase_context() to auto-fill next phase target/goal - Add validate-roadmap command to check deliverable paths exist on disk - Mark telemetry Phase 1 complete in both per-package and root ROADMAPs Closes #21, partially addresses #22
📝 WalkthroughWalkthroughUpdates roadmap files to mark telemetry Phase 1 (Storage) complete and adjust critical-path phrasing; adds a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (`forge-helper.sh`)
participant ROADMAP as ROADMAP.md (root or packages/...)
participant FS as Filesystem (deliverable paths)
participant HANDOFF as HANDOFF.md
CLI->>ROADMAP: read selected ROADMAP.md
CLI->>FS: validate checked deliverable backticked paths
alt any missing path
FS-->>CLI: missing path -> exit non-zero
else all exist
FS-->>CLI: OK
CLI->>ROADMAP: determine wave/phase via _detect_wave_for_package
CLI->>HANDOFF: update "Completed Work" table (insert row if absent)
CLI->>HANDOFF: _advance_next_phase_context -> rewrite Next Phase Context
HANDOFF-->>CLI: write complete
CLI-->>CLI: append changelog entry with wave_label
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit abfae6b
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/forge-helper.sh (1)
792-812: Consider deduplication of Completed Work entries.The awk script inserts a new row after the last table line without checking if this phase was already recorded. If
update-handoffis called multiple times for the same phase, duplicate rows will appear.This may be acceptable if the workflow guarantees single invocation per phase, but you could add a guard:
Optional: Skip insertion if phase already recorded
+ # Check if this phase is already recorded + if grep -qE "^\| ${wave_label} \| ${pkg_label} \| Phase ${phase} \|" "$handoff_file" 2>/dev/null; then + log "Phase ${phase} already recorded in Completed Work — skipping" + else local cw_row="| ${wave_label} | ${pkg_label} | Phase ${phase} | ${phase_title} |" awk -v row="$cw_row" ' /^## Completed Work/ { in_cw=1 } in_cw && /^\|/ { last_table_line=NR } in_cw && /^---$/ { in_cw=0 } { lines[NR]=$0 } END { for (i=1; i<=NR; i++) { print lines[i] if (i == last_table_line) print row } } ' "$handoff_file" > "${handoff_file}.tmp" && mv "${handoff_file}.tmp" "$handoff_file" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/forge-helper.sh` around lines 792 - 812, The awk insertion unconditionally appends cw_row and can create duplicates when update-handoff runs multiple times; modify the logic that builds/prints the table (the awk block that uses cw_row and reads "$handoff_file") to first scan the table for an existing matching row (compare full trimmed line to cw_row or match a regex using ${pkg_label} and "Phase ${phase}") and set a flag (e.g., found_existing) so the END block only prints cw_row if not found; alternatively perform a pre-check using grep for the pattern and skip the awk insertion entirely when a match exists; keep references to cw_row, the awk block, _detect_wave_for_package and handoff_file when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/forge-helper.sh`:
- Around line 792-812: The awk insertion unconditionally appends cw_row and can
create duplicates when update-handoff runs multiple times; modify the logic that
builds/prints the table (the awk block that uses cw_row and reads
"$handoff_file") to first scan the table for an existing matching row (compare
full trimmed line to cw_row or match a regex using ${pkg_label} and "Phase
${phase}") and set a flag (e.g., found_existing) so the END block only prints
cw_row if not found; alternatively perform a pre-check using grep for the
pattern and skip the awk insertion entirely when a match exists; keep references
to cw_row, the awk block, _detect_wave_for_package and handoff_file when
applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7374327-e851-4354-b846-426c9fee33bc
📒 Files selected for processing (3)
ROADMAP.mdpackages/telemetry/ROADMAP.mdscripts/forge-helper.sh
![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: 311bd127f2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- P2: add word boundary to phase regex in _detect_wave_for_package() to prevent Phase 1 from matching Phase 10 - P1: extract and validate ALL backticked paths in multi-path deliverables instead of skipping comma-separated entries - Dedup: guard against duplicate Completed Work entries in cmd_update_handoff() when called multiple times for same phase - Bonus: fix BSD sed compatibility for task_id extraction (\s → [[:space:]])
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/forge-helper.sh (1)
805-817: Minor edge case: awk row insertion relies onlast_table_linebeing set.If the "## Completed Work" section has no table rows (only headers),
last_table_linewould be set to the header row line, which should still work correctly. However, if the section is malformed or missing entirely, the row insertion silently fails.This is likely acceptable given HANDOFF.md is initialized from a template, but consider adding a warning if no insertion occurred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/forge-helper.sh` around lines 805 - 817, The current awk insertion may silently do nothing if the "## Completed Work" section is missing or malformed; after running the awk/mv block that tries to insert cw_row into handoff_file, add a post-check that verifies the row was actually inserted (for example: grep -Fq "$cw_row" "$handoff_file") and if the check fails print a warning to stderr (echo >&2) explaining insertion did not occur and include handoff_file and cw_row in the message; use the existing variables cw_row and handoff_file so you locate the code near the awk block and keep the shell exit status unchanged (only emit a warning).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/forge-helper.sh`:
- Around line 805-817: The current awk insertion may silently do nothing if the
"## Completed Work" section is missing or malformed; after running the awk/mv
block that tries to insert cw_row into handoff_file, add a post-check that
verifies the row was actually inserted (for example: grep -Fq "$cw_row"
"$handoff_file") and if the check fails print a warning to stderr (echo >&2)
explaining insertion did not occur and include handoff_file and cw_row in the
message; use the existing variables cw_row and handoff_file so you locate the
code near the awk block and keep the shell exit status unchanged (only emit a
warning).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57fb0f9c-d4f1-47a9-904e-c5798f1fe549
📒 Files selected for processing (1)
scripts/forge-helper.sh
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
cmd_update_handoff()to populate the Completed Work table, remove(none yet)placeholder, and auto-advance the Next Phase Context section to the next incomplete phasevalidate-roadmapcommand that checks completed task deliverable paths exist on disk, catching drift between ROADMAP claims and actual file structureChanges
scripts/forge-helper.sh(+205 lines)Issue #21 fixes:
_detect_wave_for_package()— scans root ROADMAP.md wave tables to find which wave a package+phase belongs to_advance_next_phase_context()— rewrites the Next Phase Context section in HANDOFF.md by finding the next incomplete phasecmd_update_handoff()— populates Completed Work table via awk insertion, removes(none yet)placeholder, auto-advances Next Phase Context, includes wave labels in entriesIssue #22 fixes:
cmd_validate_roadmap()— scans ROADMAP for checked tasks with[deliverable: \path`]`, verifies each path exists on diskshow_usage(), andmain()dispatchpackages/telemetry/ROADMAP.md[x](was[ ]), header updated to(COMPLETE)ROADMAP.mdTesting
bash -nsyntax check passesvalidate-roadmap --package telemetry→ 4/4 deliverable paths valid ✅validate-roadmap --package pipeline→ correctly detects 4 drifted paths (real refactoring drift)validate-roadmap --package mcp→ correctly detects 25 drifted paths (structure diverged from original ROADMAP)update-handoffmanually tested: Completed Work insertion ✅, placeholder removal ✅, phase auto-advance ✅npx nx affected -t build,test,lint→ all pass (33 tests, 0 lint errors)Not included (deferred)
**DATE**/__DATE__mismatch (pre-existing, out of scope)Closes #21
Partially addresses #22
Summary by CodeRabbit
New Features
Documentation