Repository navigation
feat: flat design docs + decoupled reference sync - #23
Closed
flyingrobots wants to merge 5 commits into
Closed
flyingrobots wants to merge 5 commits into
flyingrobots wants to merge 5 commits into
Conversation
Replace directory-per-cycle design docs (docs/design/<NNNN-slug>/<slug>.md) with flat files (docs/design/<LEGEND>_<slug>.md). Cycle names are now legend-prefixed slugs (e.g. PROCESS_doctor-command) instead of numbered (0039-doctor-command), removing sequential numbering that doesn't scale for concurrent users. - Remove `number` field from CycleSchema - Design docs are flat files directly in docs/design/ - Retros keep directory layout for witness subdirs - Release-scoped design docs also flatten - Doctor detects legacy nested design doc dirs - Doctor repair flattens legacy dirs and updates frontmatter - Legacy NNNN-slug layout remains discoverable for backward compat - Delete extractReleasedCycleNumbers (number-based CHANGELOG detection) - Update process.md branch naming and path conventions BREAKING CHANGE: Cycle names no longer use sequential numeric prefixes. The `number` field has been removed from the Cycle type. Design docs are now flat files instead of directory-per-cycle.
…c-coupling - Flatten design doc from legacy nested dir to docs/design/PROCESS_*.md - Fill in hill, playback questions, and contract sections - Add 5 new tests matching playback questions (drift clean) - Close cycle with hill-met outcome and retro summary
Summary by CodeRabbit
WalkthroughThis PR comprehensively refactors METHOD's backlog management, command surface, and documentation generation. It decouples reference-doc refresh from ship sync via a new Changes
Sequence Diagram(s)Generated Reference Sync vs. Ship Sync Workflow: sequenceDiagram
participant Developer
participant CLI
participant Workspace
participant FileSystem
participant BEARING as Ship Artifacts<br/>(BEARING, CHANGELOG)
participant ReferenceTarget as Reference Docs<br/>(ARCHITECTURE, CLI, MCP, GUIDE)
Developer->>CLI: method sync refs
activate CLI
CLI->>Workspace: syncRefs()
activate Workspace
Workspace->>FileSystem: read REFERENCE_DOC_TARGETS
Workspace->>ReferenceTarget: refresh generated sections
Workspace->>FileSystem: write updated targets
Workspace-->>CLI: {targets, updated}
deactivate Workspace
CLI-->>Developer: print updated targets
deactivate CLI
Developer->>CLI: method sync ship
activate CLI
CLI->>Workspace: shipSync()
activate Workspace
Workspace->>BEARING: regenerate & update
Workspace->>BEARING: regenerate CHANGELOG
Workspace->>ReferenceTarget: regenerate refs (side-effect)
Workspace-->>CLI: sync result
deactivate Workspace
CLI-->>Developer: sync confirmation
deactivate CLI
Note over ReferenceTarget,BEARING: sync refs isolates reference<br/>updates from ship-only mutations
Backlog Item Lifecycle with Dependencies: sequenceDiagram
participant User
participant CLI
participant Workspace
participant Backlog as Backlog Item
participant Dependencies
User->>CLI: method backlog add --lane asap --title "Task A"
activate CLI
CLI->>Workspace: createBacklogItem(…)
activate Workspace
Workspace->>Backlog: create with frontmatter (title, lane, owner, keywords)
Workspace-->>CLI: {path, lane, title, stem}
deactivate Workspace
deactivate CLI
User->>CLI: method backlog edit <item> --blocked-by <dep1> --blocks <dep2>
activate CLI
CLI->>Workspace: editBacklogMetadata(…)
activate Workspace
Workspace->>Backlog: update frontmatter (blocked_by, blocks arrays)
Workspace-->>CLI: {updatedFields, path}
deactivate Workspace
deactivate CLI
User->>CLI: method backlog deps <item> --critical-path
activate CLI
CLI->>Workspace: backlogDependencies({item, criticalPath: true})
activate Workspace
Workspace->>Dependencies: traverse blocked_by/blocks graph
Workspace->>Backlog: detect unblocked (ready) items
Workspace-->>CLI: {focus, edges, readyItems, criticalPath, hasCycle}
deactivate Workspace
deactivate CLI
User->>CLI: method retire <item> --reason "Duplicate" --yes
activate CLI
CLI->>Workspace: retireBacklogItem(item, reason, …)
activate Workspace
Workspace->>Backlog: move to graveyard with disposition note
Workspace-->>CLI: {ok, sourcePath, graveyardPath, reason}
deactivate Workspace
deactivate CLI
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes This PR fundamentally reshapes multiple core subsystems: domain models (lanes, cycles, frontmatter typing), workspace APIs (backlog CRUD, querying, dependency resolution), CLI/MCP command coverage (20+ new command shapes and MCP tools), reference generation (decoupling from ship sync), and test expectations across the entire suite. The changes span heterogeneous domains (documentation, parsing, types, logic, rendering, testing) with complex interdependencies (e.g., new typed frontmatter threading through frontmatter↔domain↔workspace↔query paths), dense logic in multiple areas (dependency DAG, backlog querying with multiple filter combinations, repair execution with side effects, next-work advisory scoring), and large-scale test refactoring (800+ lines). High heterogeneity and logic density demand line-by-line verification of each subsystem's contract boundaries and mutation semantics. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/method/process.md (1)
142-149:⚠️ Potential issue | 🟡 MinorInclude release-scoped cycle packets in the Executive Summary inventory.
Earlier in this doc, the default loop explicitly places some design docs and retros under
docs/releases/<version>/..., but Phase 1 only inventoriesdocs/design/*.mdanddocs/method/retro/*/. That means the summary protocol will silently ignore release-scoped cycle history.Suggested doc fix
1. Enumerate governing surfaces in precedence order: - `README.md` - repo instructions and `docs/method/process.md` - `docs/method/legends/*.md` - `docs/design/*.md` + - `docs/releases/*/design/*.md` - `docs/method/retro/*/` + - `docs/releases/*/retros/*/` - backlog lanes in repo-defined order - `docs/method/graveyard/`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/method/process.md` around lines 142 - 149, The Executive Summary inventory currently enumerates governing surfaces but omits release-scoped cycle packets; update the list under "Enumerate governing surfaces in precedence order" in docs/method/process.md to include release-scoped artifacts (e.g., add `docs/releases/<version>/...` or `docs/releases/*/` between `docs/method/retro/*/` and `docs/design/*.md` as appropriate) so release-scoped cycle history is captured during Phase 1; ensure the wording references "release-scoped cycle packets" so the summary protocol will include those files in the inventory.src/frontmatter.ts (1)
60-96:⚠️ Potential issue | 🟠 MajorCRLF frontmatter still round-trips as “no frontmatter.”
These new typed helpers inherit
parseDoc()'s LF-only delimiter detection. A doc with\r\n---\r\nfrontmatter or a closing delimiter at EOF will still parse as body-only, soreadTypedFrontmatter()returns{}andupdateTypedFrontmatter()prepends a second frontmatter block instead of updating the existing one. Now that typed metadata is the main write path, this becomes a real corruption path on Windows-authored docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontmatter.ts` around lines 60 - 96, The typed frontmatter helpers mis-detect CRLF-delimited or EOF-terminated frontmatter because parseDoc only checks for LF-only delimiters; update parseDoc (used by readTypedFrontmatter and updateTypedFrontmatter) to recognize FM_DELIMITER/FM_END with either LF or CRLF line endings and treat a closing delimiter at EOF as valid. Specifically, adjust the delimiter detection logic to accept "\n" or "\r\n" sequences when locating FM_DELIMITER and FM_END (and consider raw.endsWith(FM_END) as a valid close), so that parseDoc returns the existing frontmatter for CRLF and EOF-terminated blocks and updateTypedFrontmatter/serializeDoc will update rather than prepend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CLI.md`:
- Line 77: The phrase "Promote a backlog item into the next numbered cycle
packet." in CLI.md is inconsistent with the wording used in MCP.md; update this
sentence to match MCP.md's terminology (e.g., replace "numbered cycle packet"
with the exact phrase used in MCP.md) so both docs use the same canonical term,
and ensure the follow-up sentence referencing "docs/releases/<version>/" remains
unchanged.
In `@docs/invariants/sponsor-abstractness.md`:
- Around line 27-28: Replace the awkward sentence that currently reads "Doctrine
that says `human`, `agent`, or `user` is describing a seat or perspective in the
workflow, not a literal identity." with a direct normative form such as
"Doctrine: `human`, `agent`, and `user` describe a seat or perspective in the
workflow, not a literal identity." Update the text around the backticked terms
`human`, `agent`, and `user` to use the colon + present-tense verb construction
to improve clarity and grammar.
In `@docs/MCP.md`:
- Line 175: The docs statement "next numbered cycle packet" is outdated because
CycleSchema no longer has the number field and cycles are now named with the
LEGEND_slug pattern; update the sentence to refer to the next cycle packet by
its new naming convention (e.g., "<LEGEND>_<slug>" or "release-scoped cycle
name") and remove any implication of numeric IDs, and ensure any examples or
references to CycleSchema.number are replaced with the new naming semantics used
elsewhere in the codebase.
In `@docs/method/backlog/v1.1.0/PROCESS_doctor-repair-command.md`:
- Around line 26-37: The doc introduces a new CLI surface "method repair" and
MCP tool "method_repair" which conflicts with the established entrypoint "method
doctor --repair"; update the proposal to use the existing entrypoint (or
explicitly document "method repair" as a backwards-compatible alias) and ensure
the described flags (--plan and --apply) and the repair classes still map to
"method doctor --repair" behavior, reusing the same bounded repair set; also
reconcile any tool name references by either renaming "method_repair" to match
the existing doctor flow or adding a clear alias statement so operators and
implementers are not split.
In `@README.md`:
- Around line 174-181: The README examples still show the old nested design doc
layout; update the example mappings and any flow diagram references so they
match the flattened output paths used by the code: map
backlog/asap/SYNTH_executive-summary-protocol.md -> docs/design/<cycle>.md and
backlog/v2.4.5/PROCESS_release-scope.md ->
docs/releases/v2.4.5/design/<cycle>.md (or if releases are also flattened, use
docs/releases/v2.4.5/<cycle>.md per the new behavior), and replace the
deprecated design/<cycle>/... occurrences in the flow diagram and any repeated
examples (the two example lines in the diff and the later repeated block) with
the correct flattened paths.
In `@src/cli.ts`:
- Around line 48-69: The user-facing error/help string in the doctor module
currently recommends the non-existent syntax "method doctor --repair"; update
that string to recommend the correct commands ("method repair --plan" or "method
repair --apply") wherever it’s emitted (the message literal in the doctor module
that produces the guidance around repair usage), and update the corresponding
test assertion in the doctor tests that expects the old text so it asserts the
new, correct guidance instead. Ensure the replacement preserves surrounding
punctuation/formatting and update any test snapshots or string equality checks
that reference the old phrase.
In `@src/doctor.ts`:
- Around line 133-148: The current code maps selectedIssues into repairs using
the initial doctor snapshot, which can apply outdated targets after earlier
mutating repairs; change this to apply repairs sequentially: iterate over
selectedIssues one-by-one, call applyRepair(root, issue) for each, record
outcome and add outcome.touchedPath to touchedPaths, then immediately call
runDoctor(root) to refresh the current issues/targets before moving to the next
issue (skip or recompute any remaining issue whose repair.targetPath no longer
exists). Update the construction of the DoctorRepairAction results (currently
built in the repairs mapping) to be emitted/collected from this sequential loop
rather than from selectedIssues.map so targets reflect the latest state.
- Around line 715-745: The current flattening logic picks mdFiles[0] and then
rmSync(target) which destroys other markdowns/assets; add a guard when
mdFiles.length > 1 to avoid destructive behavior: check mdFiles count after
computing mdFiles, and if > 1 return { status: 'skipped', reason:
'ambiguous-md-files' } (or log and abort) so you do not compute
sourceFile/updated/writeFileSync/ rmSync; keep the rest of the logic for the
single-file case unchanged (references: mdFiles, sourceFile, raw, cycleName,
flatPath, updated, writeFileSync, rmSync).
In `@src/index.ts`:
- Around line 1759-1813: The recursive strongConnect inside findDependencyCycles
can overflow the call stack for very large graphs; replace the recursive Tarjan
implementation with an iterative one by simulating call frames on an explicit
stack: create a frame structure holding the current node, an iterator/index into
its adjacency list, and a flag for post-visit processing, push the initial node
frame, and loop popping/peeking frames to emulate the enter/recurse/return
behavior while maintaining indexByNode, lowlinkByNode, stack, onStack, and
components exactly as the recursive algorithm does; ensure you update lowlink
values after processing each neighbor frame and detect root-of-SCC conditions to
pop component nodes, referencing the functions/variables findDependencyCycles,
strongConnect (to be removed/rewritten), indexByNode, lowlinkByNode, stack,
onStack, and adjacency so callers and semantics remain unchanged.
In `@src/mcp.ts`:
- Around line 887-905: Add a one-line comment above the validation functions to
document the intentional asymmetry: state that validateText (and
validateOptionalText) intentionally preserves leading/trailing whitespace for
body/reason fields, while validateString (used for identifiers) trims/normalizes
input, so future refactors don’t accidentally unify their behavior; reference
the function names validateText, validateOptionalText, and validateString in the
comment for clarity.
In `@tests/domain.test.ts`:
- Around line 21-29: Add an explicit test that verifies legacy payloads
containing the removed number field are rejected: create a legacyCycle object
(based on validCycle) with a number property set (e.g., number: 1) and assert
that CycleSchema.parse(legacyCycle) throws a validation error (or otherwise
fails) to lock the removed-`number` contract; place this alongside the existing
validCycle/expect(CycleSchema.parse(validCycle)).toEqual(validCycle) assertion
so the test suite prevents silent drift.
In `@tests/ship-sync.test.ts`:
- Around line 51-53: The test hardcodes the derived backlog filename which is
fragile; change the test to capture and reuse the path returned by
workspace.captureIdea instead of the literal
'docs/method/backlog/inbox/PROCESS_asap-priority.md'. Call
workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority'), store its
return value (e.g., const capturedPath = workspace.captureIdea(...)), and pass
capturedPath into workspace.moveBacklogItem(capturedPath, 'asap') so
moveBacklogItem uses the actual generated path.
- Around line 155-156: The assertion on result.targets is order-dependent;
change it to assert set-like membership and cardinality instead: use
result.targets with an unordered membership matcher (e.g.,
expect.arrayContaining) to ensure it includes 'ARCHITECTURE.md', 'docs/CLI.md',
'docs/MCP.md', 'docs/GUIDE.md', add a length/assertion (e.g., toHaveLength(4))
to enforce cardinality, and keep the negative assertion that result.targets does
not contain 'CHANGELOG.md'; update the assertions around the result.targets
symbol in the test to these membership+cardinality checks.
---
Outside diff comments:
In `@docs/method/process.md`:
- Around line 142-149: The Executive Summary inventory currently enumerates
governing surfaces but omits release-scoped cycle packets; update the list under
"Enumerate governing surfaces in precedence order" in docs/method/process.md to
include release-scoped artifacts (e.g., add `docs/releases/<version>/...` or
`docs/releases/*/` between `docs/method/retro/*/` and `docs/design/*.md` as
appropriate) so release-scoped cycle history is captured during Phase 1; ensure
the wording references "release-scoped cycle packets" so the summary protocol
will include those files in the inventory.
In `@src/frontmatter.ts`:
- Around line 60-96: The typed frontmatter helpers mis-detect CRLF-delimited or
EOF-terminated frontmatter because parseDoc only checks for LF-only delimiters;
update parseDoc (used by readTypedFrontmatter and updateTypedFrontmatter) to
recognize FM_DELIMITER/FM_END with either LF or CRLF line endings and treat a
closing delimiter at EOF as valid. Specifically, adjust the delimiter detection
logic to accept "\n" or "\r\n" sequences when locating FM_DELIMITER and FM_END
(and consider raw.endsWith(FM_END) as a valid close), so that parseDoc returns
the existing frontmatter for CRLF and EOF-terminated blocks and
updateTypedFrontmatter/serializeDoc will update rather than prepend.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9afbbcb-5729-47ac-bc78-8ebd38c10c3d
📒 Files selected for processing (50)
ARCHITECTURE.mdREADME.mddocs/CLI.mddocs/GUIDE.mddocs/MCP.mddocs/design/0010-yaml-frontmatter-schema/yaml-frontmatter-schema.mddocs/design/PROCESS_generated-reference-sync-coupling.mddocs/invariants/sponsor-abstractness.mddocs/method/backlog/bad-code/PROCESS_generated-reference-sync-coupling.mddocs/method/backlog/cool-ideas/PROCESS_i18n-string-extraction.mddocs/method/backlog/cool-ideas/PROCESS_interactive-scaffolder.mddocs/method/backlog/cool-ideas/PROCESS_multi-forge-adapter.mddocs/method/backlog/cool-ideas/PROCESS_semantic-drift-detector.mddocs/method/backlog/v1.1.0/PROCESS_backlog-authoring-command.mddocs/method/backlog/v1.1.0/PROCESS_backlog-move-command.mddocs/method/backlog/v1.1.0/PROCESS_doctor-repair-command.mddocs/method/backlog/v1.1.0/PROCESS_feedback-command.mddocs/method/backlog/v1.1.0/PROCESS_retire-command.mddocs/method/backlog/v1.1.0/PROCESS_signpost-helper-commands.mddocs/method/backlog/v1.1.0/PROCESS_structured-repair-hints.mddocs/method/backlog/v1.1.0/PROCESS_task-dependency-dag.mddocs/method/feedback/README.mddocs/method/feedback/archive/2026-04-11-missing-api-surfaces.mddocs/method/process.mddocs/method/release.mddocs/method/retro/PROCESS_generated-reference-sync-coupling/PROCESS_generated-reference-sync-coupling.mddocs/method/retro/PROCESS_generated-reference-sync-coupling/witness/verification.mdflyingrobots-method-1.0.0.tgzsrc/adapters/github.tssrc/cli-args.tssrc/cli-renderer.tssrc/cli.tssrc/doctor.tssrc/domain.tssrc/feedback-surface.tssrc/frontmatter.tssrc/generate.tssrc/index.tssrc/mcp.tssrc/renderers.tstests/api.test.tstests/cli.test.tstests/docs.test.tstests/doctor.test.tstests/domain.test.tstests/drift.test.tstests/exec.test.tstests/mcp.test.tstests/ship-sync.test.tstests/witness.test.ts
💤 Files with no reviewable changes (1)
- docs/method/backlog/bad-code/PROCESS_generated-reference-sync-coupling.md
| ### `method pull <item>` | ||
|
|
||
| Promote a backlog item into the next numbered design cycle. | ||
| Promote a backlog item into the next numbered cycle packet. Release-tagged work scaffolds under docs/releases/<version>/. |
There was a problem hiding this comment.
Same "numbered cycle packet" inconsistency as MCP.md.
📝 Suggested fix
-Promote a backlog item into the next numbered cycle packet. Release-tagged work scaffolds under docs/releases/<version>/.
+Promote a backlog item into a new cycle packet. Release-tagged work scaffolds under docs/releases/<version>/.📝 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
| Promote a backlog item into the next numbered cycle packet. Release-tagged work scaffolds under docs/releases/<version>/. | |
| Promote a backlog item into a new cycle packet. Release-tagged work scaffolds under docs/releases/<version>/. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CLI.md` at line 77, The phrase "Promote a backlog item into the next
numbered cycle packet." in CLI.md is inconsistent with the wording used in
MCP.md; update this sentence to match MCP.md's terminology (e.g., replace
"numbered cycle packet" with the exact phrase used in MCP.md) so both docs use
the same canonical term, and ensure the follow-up sentence referencing
"docs/releases/<version>/" remains unchanged.
Comment on lines
+27
to
+28
| - Doctrine that says `human`, `agent`, or `user` is describing a seat or | ||
| perspective in the workflow, not a literal identity. |
There was a problem hiding this comment.
Tighten grammar for normative clarity.
The sentence is awkward (“says … is describing”). Use a direct normative verb to avoid ambiguity.
Proposed fix
-- Doctrine that says `human`, `agent`, or `user` is describing a seat or
- perspective in the workflow, not a literal identity.
+- Doctrine states that `human`, `agent`, and `user` describe workflow
+ seats/perspectives, not literal identities.📝 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
| - Doctrine that says `human`, `agent`, or `user` is describing a seat or | |
| perspective in the workflow, not a literal identity. | |
| - Doctrine states that `human`, `agent`, and `user` describe workflow | |
| seats/perspectives, not literal identities. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/invariants/sponsor-abstractness.md` around lines 27 - 28, Replace the
awkward sentence that currently reads "Doctrine that says `human`, `agent`, or
`user` is describing a seat or perspective in the workflow, not a literal
identity." with a direct normative form such as "Doctrine: `human`, `agent`, and
`user` describe a seat or perspective in the workflow, not a literal identity."
Update the text around the backticked terms `human`, `agent`, and `user` to use
the colon + present-tense verb construction to improve clarity and grammar.
| ### `method_pull` | ||
|
|
||
| Promote a backlog item into the next numbered cycle | ||
| Promote a backlog item into the next numbered cycle packet, using release-scoped paths when the backlog item carries release metadata. |
There was a problem hiding this comment.
Documentation says "next numbered cycle packet" but PR removes numeric cycle IDs.
The PR explicitly removes the number field from CycleSchema and switches to <LEGEND>_<slug> naming. This description is now misleading.
📝 Suggested fix
-Promote a backlog item into the next numbered cycle packet, using release-scoped paths when the backlog item carries release metadata.
+Promote a backlog item into a new cycle packet, using release-scoped paths when the backlog item carries release metadata.📝 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
| Promote a backlog item into the next numbered cycle packet, using release-scoped paths when the backlog item carries release metadata. | |
| Promote a backlog item into a new cycle packet, using release-scoped paths when the backlog item carries release metadata. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/MCP.md` at line 175, The docs statement "next numbered cycle packet" is
outdated because CycleSchema no longer has the number field and cycles are now
named with the LEGEND_slug pattern; update the sentence to refer to the next
cycle packet by its new naming convention (e.g., "<LEGEND>_<slug>" or
"release-scoped cycle name") and remove any implication of numeric IDs, and
ensure any examples or references to CycleSchema.number are replaced with the
new naming semantics used elsewhere in the codebase.
Comment on lines
+26
to
+37
| - Surface: | ||
| ship a CLI command such as | ||
| `method repair --plan` and `method repair --apply` | ||
| plus an MCP tool such as `method_repair`. | ||
| - Repair classes: | ||
| the first slice should handle safe deterministic repairs only: | ||
| missing directories, missing scaffold files, and minimal frontmatter | ||
| stubs for METHOD packet markdown. | ||
| - Planning first: | ||
| `--plan` must show the exact repairs that would be made. `--apply` | ||
| must reuse that same bounded repair set rather than inventing new | ||
| behavior on the fly. |
There was a problem hiding this comment.
Command surface conflicts with established doctor repair entrypoint.
This proposal introduces method repair ..., while the active surface in this cycle is method doctor --repair. That split will create operator and implementation drift unless explicitly framed as alias/evolution.
Proposed fix
-- Surface:
- ship a CLI command such as
- `method repair --plan` and `method repair --apply`
- plus an MCP tool such as `method_repair`.
+- Surface:
+ extend the `method doctor` repair flow with explicit planning/apply modes
+ (for example, `method doctor --repair --plan` and
+ `method doctor --repair --apply`) plus an MCP counterpart with matching
+ semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/v1.1.0/PROCESS_doctor-repair-command.md` around lines 26
- 37, The doc introduces a new CLI surface "method repair" and MCP tool
"method_repair" which conflicts with the established entrypoint "method doctor
--repair"; update the proposal to use the existing entrypoint (or explicitly
document "method repair" as a backwards-compatible alias) and ensure the
described flags (--plan and --apply) and the repair classes still map to "method
doctor --repair" behavior, reusing the same bounded repair set; also reconcile
any tool name references by either renaming "method_repair" to match the
existing doctor flow or adding a clear alias statement so operators and
implementers are not split.
Comment on lines
+174
to
181
| When a backlog item is pulled into a cycle, it becomes a design doc. | ||
| Unreleased work without explicit release scope goes to `docs/design/`. | ||
| Release-tagged work goes to `docs/releases/<version>/design/`. | ||
|
|
||
| ```text | ||
| backlog/asap/SYNTH_executive-summary-protocol.md -> design/<cycle>/executive-summary-protocol.md | ||
| backlog/v2.4.5/PROCESS_release-scope.md -> releases/v2.4.5/design/<cycle>/release-scope.md | ||
| ``` |
There was a problem hiding this comment.
Fix the packet path examples to match the flattened layout.
These examples still describe the retired .../design/<cycle>/... shape, but the new behavior writes flat design docs (docs/design/<cycle>.md and docs/releases/<version>/design/<cycle>.md). Right now the README points operators at paths that no longer exist, and the same mismatch is repeated in the flow diagram.
Also applies to: 471-474
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 174 - 181, The README examples still show the old
nested design doc layout; update the example mappings and any flow diagram
references so they match the flattened output paths used by the code: map
backlog/asap/SYNTH_executive-summary-protocol.md -> docs/design/<cycle>.md and
backlog/v2.4.5/PROCESS_release-scope.md ->
docs/releases/v2.4.5/design/<cycle>.md (or if releases are also flattened, use
docs/releases/v2.4.5/<cycle>.md per the new behavior), and replace the
deprecated design/<cycle>/... occurrences in the flow diagram and any repeated
examples (the two example lines in the diff and the later repeated block) with
the correct flattened paths.
Comment on lines
+1759
to
+1813
| function findDependencyCycles( | ||
| nodes: readonly string[], | ||
| adjacency: ReadonlyMap<string, ReadonlySet<string>>, | ||
| ): string[][] { | ||
| let index = 0; | ||
| const indexByNode = new Map<string, number>(); | ||
| const lowlinkByNode = new Map<string, number>(); | ||
| const stack: string[] = []; | ||
| const onStack = new Set<string>(); | ||
| const components: string[][] = []; | ||
|
|
||
| const strongConnect = (node: string) => { | ||
| indexByNode.set(node, index); | ||
| lowlinkByNode.set(node, index); | ||
| index += 1; | ||
| stack.push(node); | ||
| onStack.add(node); | ||
|
|
||
| for (const next of adjacency.get(node) ?? []) { | ||
| if (!indexByNode.has(next)) { | ||
| strongConnect(next); | ||
| lowlinkByNode.set(node, Math.min(lowlinkByNode.get(node)!, lowlinkByNode.get(next)!)); | ||
| } else if (onStack.has(next)) { | ||
| lowlinkByNode.set(node, Math.min(lowlinkByNode.get(node)!, indexByNode.get(next)!)); | ||
| } | ||
| } | ||
|
|
||
| if (lowlinkByNode.get(node) !== indexByNode.get(node)) { | ||
| return; | ||
| } | ||
|
|
||
| const component: string[] = []; | ||
| while (stack.length > 0) { | ||
| const current = stack.pop()!; | ||
| onStack.delete(current); | ||
| component.push(current); | ||
| if (current === node) { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| for (let number = start; number <= end; number += 1) { | ||
| released.add(number); | ||
| const hasSelfLoop = component.length === 1 && (adjacency.get(component[0])?.has(component[0]) ?? false); | ||
| if (component.length > 1 || hasSelfLoop) { | ||
| components.push(component.sort()); | ||
| } | ||
| }; | ||
|
|
||
| for (const node of nodes) { | ||
| if (!indexByNode.has(node)) { | ||
| strongConnect(node); | ||
| } | ||
| } | ||
|
|
||
| return released; | ||
| return components.sort((left, right) => left[0]!.localeCompare(right[0]!)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Correct Tarjan's SCC implementation, but watch stack depth for massive backlogs.
The recursive strongConnect function is textbook Tarjan's algorithm. For typical workspaces (<1000 backlog items), this is fine. If someone creates a pathological workspace with 10,000+ interconnected items, the recursion depth could cause stack overflow.
Consider this a latent concern rather than an immediate issue—converting to iterative with an explicit stack would be the fix if it ever surfaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 1759 - 1813, The recursive strongConnect inside
findDependencyCycles can overflow the call stack for very large graphs; replace
the recursive Tarjan implementation with an iterative one by simulating call
frames on an explicit stack: create a frame structure holding the current node,
an iterator/index into its adjacency list, and a flag for post-visit processing,
push the initial node frame, and loop popping/peeking frames to emulate the
enter/recurse/return behavior while maintaining indexByNode, lowlinkByNode,
stack, onStack, and components exactly as the recursive algorithm does; ensure
you update lowlink values after processing each neighbor frame and detect
root-of-SCC conditions to pop component nodes, referencing the
functions/variables findDependencyCycles, strongConnect (to be
removed/rewritten), indexByNode, lowlinkByNode, stack, onStack, and adjacency so
callers and semantics remain unchanged.
Comment on lines
+887
to
905
| function validateOptionalText(value: unknown, name: string): string | undefined { | ||
| if (value === undefined) { | ||
| return undefined; | ||
| } | ||
| if (typeof value !== 'string') { | ||
| throw new Error(`${name} must be a string when provided.`); | ||
| } | ||
| const normalized = value.trim(); | ||
| if (normalized.length === 0) { | ||
| return value; | ||
| } | ||
|
|
||
| function validateText(value: unknown, name: string): string { | ||
| if (typeof value !== 'string') { | ||
| throw new Error(`${name} must be a string.`); | ||
| } | ||
| if (value.trim().length === 0) { | ||
| throw new Error(`${name} must not be empty.`); | ||
| } | ||
| return normalized; | ||
| return value; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Intentional asymmetry between validateText and validateString - worth a brief comment?
validateText preserves raw content (including leading/trailing whitespace) while validateString normalizes via trim. This is the correct semantic for body/reason fields vs identifiers, but the distinction is subtle enough that a one-liner comment could prevent future "helpful" refactors that unify them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp.ts` around lines 887 - 905, Add a one-line comment above the
validation functions to document the intentional asymmetry: state that
validateText (and validateOptionalText) intentionally preserves leading/trailing
whitespace for body/reason fields, while validateString (used for identifiers)
trims/normalizes input, so future refactors don’t accidentally unify their
behavior; reference the function names validateText, validateOptionalText, and
validateString in the comment for clarity.
Comment on lines
21
to
29
| // Valid Cycle | ||
| const validCycle = { | ||
| name: '0001-test', | ||
| number: 1, | ||
| name: 'PROCESS_test', | ||
| slug: 'test', | ||
| designDoc: 'docs/design/0001-test/test.md', | ||
| retroDoc: 'docs/method/retro/0001-test/test.md', | ||
| designDoc: 'docs/design/PROCESS_test.md', | ||
| retroDoc: 'docs/method/retro/PROCESS_test/PROCESS_test.md', | ||
| }; | ||
| expect(CycleSchema.parse(validCycle)).toEqual(validCycle); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Lock the removed-number contract with an explicit test.
Right now the fixture changed, but behavior for legacy cycle payloads containing number is not explicitly asserted. Add a direct test so this breaking change can’t silently drift.
Suggested test hardening
const validCycle = {
name: 'PROCESS_test',
slug: 'test',
designDoc: 'docs/design/PROCESS_test.md',
retroDoc: 'docs/method/retro/PROCESS_test/PROCESS_test.md',
};
expect(CycleSchema.parse(validCycle)).toEqual(validCycle);
+
+ // Legacy payload should not be accepted after removing `number`
+ expect(() =>
+ CycleSchema.parse({
+ ...validCycle,
+ number: 1,
+ }),
+ ).toThrow();📝 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
| // Valid Cycle | |
| const validCycle = { | |
| name: '0001-test', | |
| number: 1, | |
| name: 'PROCESS_test', | |
| slug: 'test', | |
| designDoc: 'docs/design/0001-test/test.md', | |
| retroDoc: 'docs/method/retro/0001-test/test.md', | |
| designDoc: 'docs/design/PROCESS_test.md', | |
| retroDoc: 'docs/method/retro/PROCESS_test/PROCESS_test.md', | |
| }; | |
| expect(CycleSchema.parse(validCycle)).toEqual(validCycle); | |
| // Valid Cycle | |
| const validCycle = { | |
| name: 'PROCESS_test', | |
| slug: 'test', | |
| designDoc: 'docs/design/PROCESS_test.md', | |
| retroDoc: 'docs/method/retro/PROCESS_test/PROCESS_test.md', | |
| }; | |
| expect(CycleSchema.parse(validCycle)).toEqual(validCycle); | |
| // Legacy payload should not be accepted after removing `number` | |
| expect(() => | |
| CycleSchema.parse({ | |
| ...validCycle, | |
| number: 1, | |
| }), | |
| ).toThrow(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/domain.test.ts` around lines 21 - 29, Add an explicit test that
verifies legacy payloads containing the removed number field are rejected:
create a legacyCycle object (based on validCycle) with a number property set
(e.g., number: 1) and assert that CycleSchema.parse(legacyCycle) throws a
validation error (or otherwise fails) to lock the removed-`number` contract;
place this alongside the existing
validCycle/expect(CycleSchema.parse(validCycle)).toEqual(validCycle) assertion
so the test suite prevents silent drift.
Comment on lines
+51
to
+53
| // Create a backlog item and move it to asap | ||
| workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority'); | ||
| workspace.moveBacklogItem('docs/method/backlog/inbox/PROCESS_asap-priority.md', 'asap'); |
There was a problem hiding this comment.
Avoid hardcoding the captured backlog path.
Line 53 hardcodes a derived filename; this makes the test fragile to slug/path generation changes. Use the returned path from captureIdea().
Proposed fix
- workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority');
- workspace.moveBacklogItem('docs/method/backlog/inbox/PROCESS_asap-priority.md', 'asap');
+ const asapItemPath = workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority');
+ workspace.moveBacklogItem(asapItemPath, 'asap');📝 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
| // Create a backlog item and move it to asap | |
| workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority'); | |
| workspace.moveBacklogItem('docs/method/backlog/inbox/PROCESS_asap-priority.md', 'asap'); | |
| // Create a backlog item and move it to asap | |
| const asapItemPath = workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority'); | |
| workspace.moveBacklogItem(asapItemPath, 'asap'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ship-sync.test.ts` around lines 51 - 53, The test hardcodes the derived
backlog filename which is fragile; change the test to capture and reuse the path
returned by workspace.captureIdea instead of the literal
'docs/method/backlog/inbox/PROCESS_asap-priority.md'. Call
workspace.captureIdea('Next priority', 'PROCESS', 'ASAP Priority'), store its
return value (e.g., const capturedPath = workspace.captureIdea(...)), and pass
capturedPath into workspace.moveBacklogItem(capturedPath, 'asap') so
moveBacklogItem uses the actual generated path.
Comment on lines
+155
to
+156
| expect(result.targets).toEqual(['ARCHITECTURE.md', 'docs/CLI.md', 'docs/MCP.md', 'docs/GUIDE.md']); | ||
| expect(result.targets).not.toContain('CHANGELOG.md'); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assertion is too order-coupled for a set-like contract.
If target order is not part of the public contract, prefer membership+cardinality over exact array order to reduce flaky failures.
Proposed fix
- expect(result.targets).toEqual(['ARCHITECTURE.md', 'docs/CLI.md', 'docs/MCP.md', 'docs/GUIDE.md']);
+ expect(result.targets).toEqual(
+ expect.arrayContaining(['ARCHITECTURE.md', 'docs/CLI.md', 'docs/MCP.md', 'docs/GUIDE.md']),
+ );
+ expect(result.targets).toHaveLength(4);📝 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
| expect(result.targets).toEqual(['ARCHITECTURE.md', 'docs/CLI.md', 'docs/MCP.md', 'docs/GUIDE.md']); | |
| expect(result.targets).not.toContain('CHANGELOG.md'); | |
| expect(result.targets).toEqual( | |
| expect.arrayContaining(['ARCHITECTURE.md', 'docs/CLI.md', 'docs/MCP.md', 'docs/GUIDE.md']), | |
| ); | |
| expect(result.targets).toHaveLength(4); | |
| expect(result.targets).not.toContain('CHANGELOG.md'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ship-sync.test.ts` around lines 155 - 156, The assertion on
result.targets is order-dependent; change it to assert set-like membership and
cardinality instead: use result.targets with an unordered membership matcher
(e.g., expect.arrayContaining) to ensure it includes 'ARCHITECTURE.md',
'docs/CLI.md', 'docs/MCP.md', 'docs/GUIDE.md', add a length/assertion (e.g.,
toHaveLength(4)) to enforce cardinality, and keep the negative assertion that
result.targets does not contain 'CHANGELOG.md'; update the assertions around the
result.targets symbol in the test to these membership+cardinality checks.
![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: d168fb6cb0
ℹ️ 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".
Comment on lines
+743
to
+744
| // Remove the old directory (all files should be the single md) | ||
| rmSync(target, { recursive: true }); |
There was a problem hiding this comment.
The flatten-design-doc repair unconditionally removes the entire legacy directory after writing one flattened markdown file, so any additional files in that directory (extra notes, diagrams, attachments, etc.) are deleted during method doctor --repair. This turns a migration helper into a destructive operation for repos that stored supporting artifacts alongside the design doc; the repair should either validate a strict single-file directory shape first or migrate all contents safely before deleting.
Useful? React with 👍 / 👎.
flyingrobots
added a commit
that referenced
this pull request
Apr 14, 2026
- Tighten grammar in sponsor-abstractness invariant doc - Narrow legacy-layout detection to NNNN-slug pattern only (prevents false positives on non-cycle subdirectories) - Add defensive comment explaining repair snapshot idempotency - Add test proving CycleSchema strips legacy `number` field - Remove hardcoded backlog path in ship-sync test
Owner
Author
PR #23 Review Feedback — Resolution SummaryAll feedback from CodeRabbit and Codex has been addressed on the
All fixes are on |
Owner
Author
|
Superseded by release/v2.0.0 branch which includes this cycle's work plus 17 additional closed cycles, CI hardening, docs reorg, and all review feedback addressed. Opening a fresh PR. |
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 cycle
PROCESS_generated-reference-sync-coupling.method sync refs/Workspace.syncRefs()refreshesARCHITECTURE.md,docs/CLI.md,docs/MCP.md, anddocs/GUIDE.mdwithout touching ship-only artifacts (CHANGELOG.md,docs/BEARING.md). Exposed via CLI, MCP (method_sync_refs), and integrated intoshipSync().docs/design/<LEGEND>_<slug>.md) instead of directory-per-cycle (docs/design/<NNNN>-<slug>/<slug>.md). Sequential numeric prefixes replaced with legend-prefixed slugs that scale for concurrent users. Retros keep directory layout for witness subdirs. Legacy layout remains discoverable.method doctordetects legacy nested design doc directories andmethod doctor --repairflattens them, updating frontmatter.BREAKING CHANGE:
numberfield removed fromCycletype. Design docs are flat files. Cycle names use<LEGEND>_<slug>format instead of<NNNN>-<slug>.Test plan
npm test)npm run build)method drift)method doctor --repairto flatten existing 39 legacy design doc dirsmain