Repository navigation
feat(dev-workflow-v2): add workflow definition domain with event-sourced state machine - #274
Merged
Merged
Conversation
…ced state machine Implement workflow-definition module using @ntcoding/agentic-workflow-builder. 10-state machine (IMPLEMENTING through COMPLETE + BLOCKED) with transition guards, operation gates, bash command filtering, and event sourcing via fold. 185 tests, 100% coverage across all domain files. Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Move test fixtures to domain/fixtures/ (SoC-010: co-locate by change) - Move output-messages to infra/ (SoC-001: CLI output formatting) - Move workflow-adapter to infra/ (SoC-001: adapter pattern) - Configure vitest coverage output to test-output/ (eslint-ignored) Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces dev-workflow-v2, a new event-sourced state machine plugin for Claude Code that enforces a structured task lifecycle. Includes domain layer (state definitions, event schemas, workflow engine), infrastructure (CLI adapters, git integration), comprehensive tests, agent definitions, and documentation. Updates workspace configuration and CI workflows to support the new tool. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI User
participant Engine as WorkflowEngine
participant Workflow as Workflow Instance
participant Registry as State Registry
participant Fold as Event Fold
CLI->>Engine: init/transaction(command)
Engine->>Workflow: record* (issue/branch/verify/etc)
Workflow->>Registry: getStateDefinition()
Registry-->>Workflow: ConcreteStateDefinition
Workflow->>Workflow: checkPreconditions()
Workflow->>Fold: applyEvent(state, event)
Fold-->>Workflow: updatedState
Workflow->>Registry: validateTransition()
Registry-->>Workflow: pass/fail
Workflow-->>Engine: PreconditionResult
Engine-->>CLI: OperationResult {output, exitCode}
CLI-->>CLI: Display result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff spans 100+ files with heterogeneous changes: comprehensive domain model (type system, event schemas, state machines, fold logic), 10 state definitions with similar but distinct constraints, infrastructure layer (CLI adapters, git integration, hook handlers), three layers of tests (units, integration, fixtures), and documentation. While individual state definitions follow repetitive patterns, the domain logic requires careful scrutiny of state guards, transitions, and event handling across multiple interacting components. Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
…rage config Add 10 state instruction markdown files referenced by agentInstructions fields in state definitions. Each file provides TODO checklists and constraints for the agent when operating in that workflow state. Also fix vitest coverage output to test-output/ (eslint-ignored path). Co-Authored-By: Claude Opus 4.6 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pnpm-workspace.yaml`:
- Line 8: The pnpm-workspace.yaml entry for onlyBuiltDependencies is using a
scalar; change it to a YAML array (e.g., onlyBuiltDependencies: [better-sqlite3,
...]) and consolidate the allow-list so workspace-wide policy lives only in
pnpm-workspace.yaml; copy the items currently in package.json's
onlyBuiltDependencies (["@swc/core","esbuild","nx","unrs-resolver"]) into the
workspace array and remove the onlyBuiltDependencies field from package.json to
avoid the union/duplication.
In `@tools/dev-workflow-v2/package.json`:
- Line 2: The package.json "name" value is unscoped ("dev-workflow-v2"); update
it to use the repository scope by renaming the package to the monorepo
convention (for example "@living-architecture/dev-workflow-v2") so cross-project
dependencies use the scoped package name consistently; locate the "name" field
in package.json and replace the unscoped string "dev-workflow-v2" with the
scoped "@living-architecture/dev-workflow-v2".
- Line 11: The GitHub dependency for "@ntcoding/agentic-workflow-builder" is
pointing at the default branch; update the package.json dependency string
(currently
"github:NTCoding/autonomous-claude-agent-team#path:packages/agentic-workflow-builder")
to pin it to an immutable ref by appending a tag or commit SHA before the &path
segment (e.g. change to
"github:NTCoding/autonomous-claude-agent-team#v1.0.0&path:packages/agentic-workflow-builder"
or use a commit SHA), commit the updated manifest so future lockfile refreshes
cannot pull a different revision.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/states/implementing.ts`:
- Around line 20-24: The fields verifyPassed, reviewPassed, and ciPassed
currently reset to boolean false in implementing.onEntry which conflates
"pending" and "failed"; change these to an explicit status enum or discriminated
union (e.g., "pending" | "passed" | "failed" or a {status: ..., finalized:
boolean} shape) and update the implementing.onEntry initializer to set them to
"pending" (or the equivalent initial union variant); then update the
guards/consumers in verifying.ts, reviewing.ts, and awaiting-ci.ts to check the
new status union instead of treating false as failure/completion so the workflow
can distinguish pending vs failed outcomes.
In
`@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.ts`:
- Around line 8-341: The tests only assert result.type, so update each
happy-path case that calls WORKFLOW_EVENT_SCHEMA.parse(...) to also assert the
event-specific fields are present and preserved (e.g. for 'issue-recorded'
assert result.issueNumber === 42; for 'branch-recorded' assert result.branch ===
'feature/foo'; for 'verify-completed' assert result.passed and when failed
assert result.output; for 'review-completed' assert result.passed and when
failed assert result.failedReviewers; for 'pr-recorded' assert result.prNumber
and optional result.prUrl; for 'ci-completed' assert result.passed and optional
result.output; for 'feedback-checked' assert result.clean and when dirty assert
result.unresolvedCount; for 'feedback-addressed' assert result.at exists; for
'reflection-written' assert result.path; for 'bash-checked' assert
result.command/allowed and optional result.reason; for 'write-checked' assert
result.filePath/allowed and optional result.reason; for 'transitioned' assert
result.from/result.to and optional result.preBlockedState; and for
'session-started' assert optional result.repository when provided) so each test
verifies the specific property survived parsing.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.ts`:
- Around line 3-91: The schemas currently accept invalid domain values; tighten
them by: replacing free-form timestamps (fields named "at") with an ISO-8601
validated string (use a zod refine/preprocess with an ISO check) across
SESSION_STARTED_SCHEMA, TRANSITIONED_SCHEMA, etc.; constrain
TRANSITIONED_SCHEMA.from and .to to the workflow state enum (use z.enum or a
shared WORKFLOW_STATE_SCHEMA) instead of z.string(); enforce positive integers
with .int().positive() for ISSUE_RECORDED_SCHEMA.issueNumber and
PR_RECORDED_SCHEMA.prNumber and optional counts
(FEEDBACK_CHECKED_SCHEMA.unresolvedCount); require non-empty strings for
paths/URLs in REFLECTION_WRITTEN_SCHEMA.path and PR_RECORDED_SCHEMA.prUrl, and
ensure arrays like REVIEW_COMPLETED_SCHEMA.failedReviewers contain non-empty
strings; apply .nonempty() where appropriate (e.g., branch, tool, command,
filePath) and tighten boolean/optional outputs in VERIFY_COMPLETED_SCHEMA and
CI_COMPLETED_SCHEMA as needed so invalid timestamps, negative/fractional ids,
empty paths, or unknown states are rejected at parse-time.
In
`@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-implementing-verifying.spec.ts`:
- Around line 124-139: The two tests seed only false flags so they don't
actually verify clearing behavior; update the "resets verify and review flags on
entry" spec (the test that currently uses eventsToVerifying(), verifyFailed(),
transitioned('VERIFYING','IMPLEMENTING')) to first seed at least one of the
relevant flags as true (for example by adding the event helper that sets
verifyPassed or reviewPassed to true before the transitioned(...) call), then
perform the transition and assert the flag was cleared; do the same for the
"resets feedback flags on entry" test by seeding feedbackClean and/or
feedbackAddressed to true (via the appropriate event helper) before calling
transitioned('VERIFYING','IMPLEMENTING') so the assertions actually observe the
reset.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-predicates.ts`:
- Around line 20-24: The current loop over DANGEROUS_FLAGS uses
command.includes(flag) which can match substrings inside paths or values; update
the check in the loop that calls fail(...) so it only matches whole flags (e.g.,
use a RegExp with word boundaries like new
RegExp(`\\b${escapeForRegex(flag)}\\b`) against the command string, or split
command into argv tokens and test equality/prefixes), ensuring you reference
DANGEROUS_FLAGS, the command variable, and the existing fail(...) call so only
true flag occurrences trigger the fail.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.spec.ts`:
- Around line 8-10: Replace the STATE_NAMES.forEach callback with a for...of
loop so the test iterates correctly without returning from an iterator callback:
loop over STATE_NAMES using "for (const s of STATE_NAMES)" and inside the loop
call expect(STATE_NAME_SCHEMA.parse(s)).toStrictEqual(s); update the test block
(the it('accepts all valid state names')) to use that for...of construct instead
of forEach.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.ts`:
- Around line 24-39: The schema currently allows preBlockedState to be any
string and the exported WorkflowState type widens state fields to string; update
createWorkflowStateSchema so preBlockedState uses stateNameSchema.optional()
(like currentStateMachineState) and ensure the exported WorkflowState type uses
the StateName type for both currentStateMachineState and preBlockedState so
invalid state names are rejected at runtime and unrepresentable at compile time;
modify the type/export for WorkflowState to reference StateName rather than
string and keep stateNameSchema as the single source of truth.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts`:
- Around line 243-245: Replace the direct parse call in transitionTo to validate
the target with STATE_NAME_SCHEMA.safeParse() instead of parseStateName(target);
if safeParse() fails, return a failed PreconditionResult via fail() with a clear
message about the invalid target, otherwise extract the parsed value and
continue using it (keep the existing
parseStateName(this.state.currentStateMachineState) usage for the current
state). Ensure you reference the parsed target value (from safeParse().data)
where parseStateName(target) was used and that any error path returns fail(...)
rather than throwing.
- Around line 42-43: createFresh currently passes the shared mutable EMPTY_STATE
into the Workflow constructor and getState returns the internal state directly,
exposing mutable references; change createFresh (and the Workflow constructor if
it directly assigns) to pass a defensive copy of EMPTY_STATE (deep clone or
Object.freeze a newly created object) and update getState to return a
deep/shallow copy (or a frozen snapshot) instead of the internal state reference
so callers cannot mutate the workflow state without raising events; refer to
createFresh, Workflow constructor, and getState to implement these defensive
copies.
- Around line 79-88: recordIssue and recordPr currently accept any number and
append invalid values, breaking WORKFLOW_STATE_SCHEMA; add input validation in
both methods to ensure the provided number is a positive integer (use
Number.isInteger(value) && value > 0 and guard against NaN) before calling
this.append(), and if validation fails return a non-passing PreconditionResult
(same shape as checkOperationGate failures) instead of appending; update
recordIssue (and recordPr) to perform this check right after the operation gate
check and only call this.append() when the value is valid so
githubIssue/prNumber always meet the schema constraints.
In `@tools/dev-workflow-v2/tsconfig.json`:
- Around line 1-19: Replace the single tsconfig.json with the required 3-file
structure: keep a minimal root tsconfig.json that sets "files": [], "references"
or simply extends per-guideline and delegates to tsconfig.lib.json and
tsconfig.spec.json; add tsconfig.lib.json containing the build-facing
compilerOptions (target/module/moduleResolution/outDir/rootDir, declaration true
if library, no tests, include "src/**/*", exclude "dist","**/*.spec.ts") and
ensure it preserves strict/noUncheckedIndexedAccess/exactOptionalPropertyTypes;
add tsconfig.spec.json for tests with test-friendly compilerOptions (include
types like "vitest/globals","node", include test files pattern such as
"src/**/*.spec.ts" and any mocks) and make sure "skipLibCheck" and
"resolveJsonModule" match project needs; update any "types" or
"include"/"exclude" entries from the original tsconfig.json into the appropriate
lib/spec files so the build and tests resolve correctly.
- Around line 1-19: Update tools/dev-workflow-v2/tsconfig.json to extend
tsconfig.base.json and merge/override only needed options: add "extends":
"tsconfig.base.json" at the top, then ensure compilerOptions includes the
missing build flags (composite, declarationMap, isolatedModules, importHelpers),
the stricter checks (noImplicitOverride, noImplicitReturns, noUnusedLocals,
noUnusedParameters, noPropertyAccessFromIndexSignature), align lib/target with
the base (use ES2023 or inherit), and add customConditions for
"@living-architecture/source"; keep existing options (module, moduleResolution,
types, etc.) but prefer inheriting common settings from the base so the package
follows monorepo standards.
- Line 12: Remove the redundant "esModuleInterop" compiler option since
"verbatimModuleSyntax" is enabled in this tsconfig; locate the options block
containing verbatimModuleSyntax and esModuleInterop and delete the
esModuleInterop entry so the config relies on verbatimModuleSyntax as in the
base config, and run a quick typecheck to confirm no downstream code depends on
esModuleInterop semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 87c287e2-66e4-4e78-ad1e-e84865133edb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
package.jsonpnpm-workspace.yamltools/dev-workflow-v2/.gitignoretools/dev-workflow-v2/package.jsontools/dev-workflow-v2/src/workflow-definition/domain/fixtures/workflow-test-fixtures.tstools/dev-workflow-v2/src/workflow-definition/domain/fold.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/fold.tstools/dev-workflow-v2/src/workflow-definition/domain/registry.tstools/dev-workflow-v2/src/workflow-definition/domain/states/addressing-feedback.tstools/dev-workflow-v2/src/workflow-definition/domain/states/awaiting-ci.tstools/dev-workflow-v2/src/workflow-definition/domain/states/blocked.tstools/dev-workflow-v2/src/workflow-definition/domain/states/checking-feedback.tstools/dev-workflow-v2/src/workflow-definition/domain/states/complete.tstools/dev-workflow-v2/src/workflow-definition/domain/states/implementing.tstools/dev-workflow-v2/src/workflow-definition/domain/states/reflecting.tstools/dev-workflow-v2/src/workflow-definition/domain/states/reviewing.tstools/dev-workflow-v2/src/workflow-definition/domain/states/submitting-pr.tstools/dev-workflow-v2/src/workflow-definition/domain/states/verifying.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-feedback-reflecting.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-hook-checks.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-implementing-verifying.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-predicates.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-reviewing-submitting.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow.tstools/dev-workflow-v2/src/workflow-definition/index.tstools/dev-workflow-v2/src/workflow-definition/infra/output-messages.spec.tstools/dev-workflow-v2/src/workflow-definition/infra/output-messages.tstools/dev-workflow-v2/src/workflow-definition/infra/workflow-adapter.spec.tstools/dev-workflow-v2/src/workflow-definition/infra/workflow-adapter.tstools/dev-workflow-v2/tsconfig.jsontools/dev-workflow-v2/vitest.config.mts
|
|
||
| autoInstallPeers: true | ||
|
|
||
| onlyBuiltDependencies: better-sqlite3 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pnpm workspace.yaml onlyBuiltDependencies scalar array format v10
💡 Result:
In pnpm v10, onlyBuiltDependencies in pnpm-workspace.yaml is a YAML list/array (not a scalar string). Only packages in this list are allowed to run preinstall/install/postinstall during installation. [1]
Valid YAML formats:
onlyBuiltDependencies:
- esbuild
- sharpor flow-style:
onlyBuiltDependencies: [esbuild, sharp]pnpm can also allow specific versions (supported as documented in v10.19.0+):
onlyBuiltDependencies:
- [email protected]
- [email protected] || 21.6.5Sources: [1], [2]
References
- [1] pnpm settings (
pnpm-workspace.yaml) –onlyBuiltDependenciesdescription and examples (pnpm.cn) - [2]
pnpm approve-builds– notes that approvals are added to theonlyBuiltDependenciesarray (pnpm.cn)
Citations:
🏁 Script executed:
# Check the actual content of pnpm-workspace.yaml around line 8
fd -e yaml -e yml | grep -E "(pnpm-workspace|workspace)" | head -5Repository: NTCoding/living-architecture
Length of output: 91
🏁 Script executed:
# Find and examine pnpm-workspace.yaml
find . -name "pnpm-workspace.yaml" -o -name "pnpm-workspace.yml" | head -5Repository: NTCoding/living-architecture
Length of output: 93
🏁 Script executed:
# Look for package.json in root to verify onlyBuiltDependencies config
cat -n package.json | grep -A 10 -B 2 "onlyBuiltDependencies"Repository: NTCoding/living-architecture
Length of output: 398
🏁 Script executed:
# Get the actual content of pnpm-workspace.yaml
cat -n pnpm-workspace.yamlRepository: NTCoding/living-architecture
Length of output: 238
🌐 Web query:
pnpm onlyBuiltDependencies package.json vs pnpm-workspace.yaml precedence merge
💡 Result:
-
Both are supported, but they live in different “layers”:
package.json→pnpm.onlyBuiltDependencies(project manifest field). [1]pnpm-workspace.yaml→onlyBuiltDependencies(pnpm settings / workspace manifest setting). [2]
-
Precedence / merge behavior (what pnpm effectively does):
- There isn’t a simple “one overrides the other” rule documented for these two fields; in practice, you can end up with both being applied, i.e. an effective union/combined allow-list, which can lead to surprising behavior if you duplicate entries in both places (including reports of scripts being run twice when mirrored in both). [3]
-
Practical guidance:
- In a monorepo, prefer defining the allow-list once in
pnpm-workspace.yaml(workspace-wide policy) and avoid also settingpnpm.onlyBuiltDependenciesin package manifests unless you have a specific reason to scope it per-package. [2]
- In a monorepo, prefer defining the allow-list once in
Sources:
[1] pnpm package.json field pnpm.onlyBuiltDependencies docs (mirror) (dimava.github.io)
[2] pnpm workspace settings onlyBuiltDependencies docs (pnpm.cn)
[3] pnpm issue describing inconsistent behavior / duplication when both are set (github.com)
Citations:
- 1: https://dimava.github.io/pnpm/package_json/
- 2: https://www.pnpm.cn/settings
- 3: Workspace onlyBuiltDependencies inconsistency pnpm/pnpm#9402
🌐 Web query:
pnpm-workspace.yaml scalar string onlyBuiltDependencies invalid error v10
💡 Result:
In pnpm v10, onlyBuiltDependencies in pnpm-workspace.yaml must be a YAML array of strings (string[]), not a single scalar string. If you set it like onlyBuiltDependencies: esbuild, pnpm’s workspace-manifest validation can fail with an error that mentions “scalar string” (because it expected a list). (newreleases.io)
Use one of these valid forms:
# pnpm-workspace.yaml
onlyBuiltDependencies:
- esbuild
- fuse-nativeor:
onlyBuiltDependencies: [esbuild, fuse-native]This list is also what pnpm approve-builds updates (it adds approved packages into onlyBuiltDependencies). (pnpm.nodejs.cn)
Citations:
- 1: https://newreleases.io/project/github/pnpm/pnpm/release/v10.5.0
- 2: https://pnpm.nodejs.cn/next/cli/approve-builds/?utm_source=openai
Fix onlyBuiltDependencies to use array format required by pnpm v10.
Line 8 uses a scalar string (onlyBuiltDependencies: better-sqlite3), but pnpm v10 requires this field in pnpm-workspace.yaml to be a YAML array. The current syntax violates the workspace-manifest schema and will cause validation errors.
Additionally, package.json Lines 85-89 already define onlyBuiltDependencies as an array with ["@swc/core", "esbuild", "nx", "unrs-resolver"]. Both settings apply as a union, which can lead to unexpected behavior. Consolidate the allow-list in pnpm-workspace.yaml (workspace-wide policy) and remove the duplicate from package.json.
Suggested fix
-onlyBuiltDependencies: better-sqlite3
+onlyBuiltDependencies:
+ - "@swc/core"
+ - esbuild
+ - nx
+ - unrs-resolver
+ - better-sqlite3📝 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
| onlyBuiltDependencies: better-sqlite3 | |
| onlyBuiltDependencies: | |
| - "@swc/core" | |
| - esbuild | |
| - nx | |
| - unrs-resolver | |
| - better-sqlite3 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pnpm-workspace.yaml` at line 8, The pnpm-workspace.yaml entry for
onlyBuiltDependencies is using a scalar; change it to a YAML array (e.g.,
onlyBuiltDependencies: [better-sqlite3, ...]) and consolidate the allow-list so
workspace-wide policy lives only in pnpm-workspace.yaml; copy the items
currently in package.json's onlyBuiltDependencies
(["@swc/core","esbuild","nx","unrs-resolver"]) into the workspace array and
remove the onlyBuiltDependencies field from package.json to avoid the
union/duplication.
Owner
Author
There was a problem hiding this comment.
Not actioned. The onlyBuiltDependencies is already in valid YAML array format. The contradictory config with ignoredBuiltDependencies in root package.json (item #40) requires CI testing to resolve safely — deferring to avoid breaking the build pipeline.
There was a problem hiding this comment.
@NTCoding, understood — if the file already has onlyBuiltDependencies in proper YAML array format, then my concern about the scalar syntax doesn't apply. Apologies for the noise.
Deferring the consolidation of the allow-lists (onlyBuiltDependencies in pnpm-workspace.yaml vs. ignoredBuiltDependencies in root package.json) until it can be safely validated through CI makes sense. No further action needed here.
🧠 Learnings used
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-07T18:39:04.966Z
Learning: Applies to **/package.json : Add workspace dependencies with living-architecture package names to package.json when importing between projects
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-07T18:39:04.966Z
Learning: Applies to **/*.{ts,tsx} : Use living-architecture scoped package names and cross-project imports with package names (not relative paths)
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 212
File: tools/dev-workflow/platform/domain/branch-naming/issue-branch-parser.spec.ts:1-26
Timestamp: 2026-01-28T13:20:46.739Z
Learning: In NTCoding/living-architecture repository: The project has a policy to not address nitpick-level review comments. Avoid making nitpick suggestions or mark them clearly as optional/informational only.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 233
File: packages/riviere-cli/src/features/extract/commands/git-changed-files.ts:56-59
Timestamp: 2026-01-31T09:36:38.865Z
Learning: In tools/dev-workflow/**/*.ts and packages/**/*.ts: Avoid 'as' type assertions and other type escape hatches as they violate project design principles.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/get-pr-feedback.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 120
File: docs/workflow/parallel-workflow.md:160-281
Timestamp: 2026-01-10T19:16:54.359Z
Learning: In the NTCoding/living-architecture repository, design and investigation documents (such as those in docs/workflow/) can reference scripts, commands, or features that don't yet exist in the codebase. These references serve as specifications for future implementation tasks, not documentation of current capabilities. This is acceptable when the PR scope is research/design rather than implementation.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 205
File: tools/dev-workflow/vitest.config.mts:18-21
Timestamp: 2026-01-21T08:49:07.503Z
Learning: In tools/dev-workflow, coverage enforcement is intentionally scoped to only tested modules (e.g., features/claude-hooks/domain/handlers, features/complete-task/domain/task-check-marker.ts). The majority of platform/ and features/ code is tested through integration testing of the complete-task pipeline with manual verification. Unit tests for thin wrapper code in this package are considered low-value relative to maintenance cost.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/respond-to-feedback.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
| "typecheck": "tsc --noEmit" | ||
| }, | ||
| "dependencies": { | ||
| "@ntcoding/agentic-workflow-builder": "github:NTCoding/autonomous-claude-agent-team#path:packages/agentic-workflow-builder", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pnpm GitHub dependency #path subdirectory pin commit SHA tag syntax package.json
💡 Result:
pnpm supports Git (incl. GitHub) dependency specifiers where:
#<commit-ish>selects a branch, tag, or commit SHA (aka “commit-ish”).&path:<subdir>(or#path:<subdir>if you’re only specifying a path) selects a package located in a subdirectory of the repo.
pnpm added explicit support for installing from a Git repo subdirectory viapath:. [1]
pnpm also documents that Git deps can pin branch/commit/tag using#. [2]
package.json examples (GitHub shorthand)
1) Subdirectory only (default branch):
{
"dependencies": {
"my-pkg": "github:user/repo#path:packages/my-pkg"
}
}[1]
2) Pin a branch/tag/commit AND pick a subdirectory:
{
"dependencies": {
"my-pkg": "github:user/repo#<branch-or-tag-or-sha>&path:packages/my-pkg"
}
}Example shown for a branch with subdir: #dev&path:.... [1]
3) Pin a commit SHA (recommended for reproducibility):
{
"dependencies": {
"my-pkg": "github:user/repo#9fceb02f3f2c2a0a1c0d5c8d9e1f2a3b4c5d6e7f&path:packages/my-pkg"
}
}(Commit SHA is a valid “commit-ish” per pnpm’s # syntax.) [2]
Equivalent full Git URL form
{
"dependencies": {
"my-pkg": "git+https://github.com/user/repo.git#<commit-ish>&path:packages/my-pkg"
}
}(Commit-ish selection via # is the same idea.) [2]
Notes
- Use
#path:...when only specifying the subdirectory; use#<commit-ish>&path:...when you want both. [1]
Sources: [1] [2]
🏁 Script executed:
# Search for other GitHub dependencies in the repository to understand pinning patterns
find . -name "package.json" -type f | head -20 | xargs grep -h "github:" | head -30Repository: NTCoding/living-architecture
Length of output: 200
🏁 Script executed:
# Check the specific file and surrounding context
cat -n tools/dev-workflow-v2/package.json | head -30Repository: NTCoding/living-architecture
Length of output: 714
🏁 Script executed:
# Search for all GitHub dependencies across the repository
find . -name "package.json" -type f -exec grep -l "github:" {} \; 2>/dev/null | xargs -I {} bash -c 'echo "=== {} ===" && grep "github:" {} | head -10'Repository: NTCoding/living-architecture
Length of output: 2120
🏁 Script executed:
# Look for any documentation or patterns about dependency management
find . -name "*.md" -type f | xargs grep -l "github:" 2>/dev/null | head -5Repository: NTCoding/living-architecture
Length of output: 302
🏁 Script executed:
# Check if there are any other dependencies from autonomous-claude-agent-team
grep -r "autonomous-claude-agent-team" . --include="*.json" --include="*.md" 2>/dev/nullRepository: NTCoding/living-architecture
Length of output: 237
Pin the GitHub dependency to an immutable ref.
Line 11 points at the default branch of a GitHub source. Any lockfile refresh can silently pull a different @ntcoding/agentic-workflow-builder revision and change behavior without a manifest diff. Update the dependency to use a pinned tag or commit SHA in the format github:NTCoding/autonomous-claude-agent-team#<tag-or-sha>&path:packages/agentic-workflow-builder (for example, #v1.0.0&path:packages/agentic-workflow-builder if a semantic tag is available).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/package.json` at line 11, The GitHub dependency for
"@ntcoding/agentic-workflow-builder" is pointing at the default branch; update
the package.json dependency string (currently
"github:NTCoding/autonomous-claude-agent-team#path:packages/agentic-workflow-builder")
to pin it to an immutable ref by appending a tag or commit SHA before the &path
segment (e.g. change to
"github:NTCoding/autonomous-claude-agent-team#v1.0.0&path:packages/agentic-workflow-builder"
or use a commit SHA), commit the updated manifest so future lockfile refreshes
cannot pull a different revision.
Owner
Author
There was a problem hiding this comment.
Rejected. Internal dev tool — branch ref is acceptable. SHA pinning adds friction for updates with no meaningful security benefit in this context.
There was a problem hiding this comment.
@NTCoding, understood — acknowledged and noted for future reviews.
✏️ Learnings added
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 274
File: tools/dev-workflow-v2/package.json:11-11
Timestamp: 2026-03-07T20:51:16.049Z
Learning: In NTCoding/living-architecture, GitHub source dependencies for internal dev tools (e.g., `ntcoding/agentic-workflow-builder` in `tools/dev-workflow-v2/package.json`) are intentionally left unpinned, pointing at a branch ref rather than a commit SHA or tag. SHA/tag pinning is considered unnecessary friction for internal tooling in this repository. Do not flag unpinned GitHub source dependencies for internal dev tools as issues.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 233
File: packages/riviere-cli/src/features/extract/commands/git-changed-files.ts:56-59
Timestamp: 2026-01-31T09:36:38.865Z
Learning: In tools/dev-workflow/**/*.ts and packages/**/*.ts: Avoid 'as' type assertions and other type escape hatches as they violate project design principles.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-07T18:39:04.966Z
Learning: Applies to **/package.json : Add workspace dependencies with living-architecture package names to package.json when importing between projects
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 245
File: package.json:8-8
Timestamp: 2026-02-01T10:56:02.381Z
Learning: The `build:deploy` script in the root package.json is a deployment-specific assembly script that intentionally uses direct tool invocations (npx tsc, npx vitepress, npx vite) instead of Nx commands because deployment assembly has different requirements than dev builds. This script copies files, builds VitePress, and assembles the dist folder, and should not be refactored to use Nx targets.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 205
File: tools/dev-workflow/vitest.config.mts:18-21
Timestamp: 2026-01-21T08:49:07.503Z
Learning: In tools/dev-workflow, coverage enforcement is intentionally scoped to only tested modules (e.g., features/claude-hooks/domain/handlers, features/complete-task/domain/task-check-marker.ts). The majority of platform/ and features/ code is tested through integration testing of the complete-task pipeline with manual verification. Unit tests for thin wrapper code in this package are considered low-value relative to maintenance cost.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 227
File: tools/dev-workflow/platform/domain/workflow-execution/run-workflow.ts:14-23
Timestamp: 2026-01-30T22:55:48.964Z
Learning: In tools/dev-workflow/platform/domain/workflow-execution/run-workflow.ts: Type aliases and interfaces (ContextBuilder, ResultFormatter, WorkflowOptions) used only in exported function signatures should remain unexported per YAGNI principle. TypeScript can inline these in declaration files without explicit exports. Only export if actual TypeScript declaration emit errors occur.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 186
File: eslint.config.mjs:43-43
Timestamp: 2026-01-17T10:27:48.337Z
Learning: The file tools/dev-workflow/external-clients/github.ts is intentionally ignored in eslint.config.mjs because it's a tooling file where ESLint checks are not needed.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 186
File: tools/dev-workflow/get-pr-feedback/steps/fetch-feedback.ts:7-20
Timestamp: 2026-01-17T13:19:34.131Z
Learning: In tools/dev-workflow: command-specific types (e.g., PRStatus in fetch-feedback.ts) should remain internal to their command directory and not be exported. This enforces the architecture principle that types scoped to a specific command/workflow step should not be shared outside that command's directory.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/complete-task.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/respond-to-feedback.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 80
File: .github/workflows/semgrep.yml:25-25
Timestamp: 2026-01-08T21:31:40.779Z
Learning: The living-architecture repository prefers semantic version tags (e.g., v4) over commit SHA pinning for GitHub Actions in workflow files, as this is the standard practice across the codebase and reduces maintenance overhead for trusted first-party GitHub Actions.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 186
File: tools/dev-workflow/external-clients/github.ts:112-137
Timestamp: 2026-01-17T13:32:39.104Z
Learning: In tools/dev-workflow/external-clients/github.ts: Do not add input validation for parameters that will be validated by the external API (e.g., GitHub API). Redundant validation provides no practical benefit when the API returns clear error messages.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 0
File: :0-0
Timestamp: 2026-01-30T13:20:59.305Z
Learning: In NTCoding/living-architecture: CodeRabbit has a recurring bug where it submits APPROVED reviews followed by CHANGES_REQUESTED reviews within seconds (e.g., PR `#226`: approved at 13:07:33, changes requested at 13:07:59). The user has `request_changes_workflow: true` correctly configured and wants PRs blocked when changes are needed. The issue is NOT the configuration - it's that CodeRabbit submits premature approvals before completing analysis, then contradicts itself with changes requested. This is a defect in CodeRabbit's review sequencing, not a configuration problem.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 212
File: tools/dev-workflow/platform/domain/branch-naming/issue-branch-parser.spec.ts:1-26
Timestamp: 2026-01-28T13:20:46.739Z
Learning: In NTCoding/living-architecture repository: The project has a policy to not address nitpick-level review comments. Avoid making nitpick suggestions or mark them clearly as optional/informational only.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-07T18:39:04.966Z
Learning: Applies to **/vitest.config.ts : Add vitest.config.ts with 100% coverage as default for new projects that need tests
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/get-pr-feedback.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 67
File: packages/riviere-extract-ts/tsconfig.spec.json:5-22
Timestamp: 2026-01-08T12:57:19.167Z
Learning: In tsconfig.spec.json files, use expanded types array ["vitest/globals", "vitest/importMeta", "vite/client", "node", "vitest"] for complete test type coverage. Include patterns should cover .test and .spec files, config files (vitest.config.ts/mts, vite.config.ts/mts), multiple extensions (.ts, .tsx, .js, .jsx), and .d.ts files to ensure proper TypeScript coverage across the testing environment.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 120
File: docs/workflow/parallel-workflow.md:160-281
Timestamp: 2026-01-10T19:16:54.359Z
Learning: In the NTCoding/living-architecture repository, design and investigation documents (such as those in docs/workflow/) can reference scripts, commands, or features that don't yet exist in the codebase. These references serve as specifications for future implementation tasks, not documentation of current capabilities. This is acceptable when the PR scope is research/design rather than implementation.
Comment on lines
+20
to
+24
| onEntry: (state: WorkflowState): WorkflowState => ({ | ||
| ...state, | ||
| verifyPassed: false, | ||
| reviewPassed: false, | ||
| ciPassed: false, |
There was a problem hiding this comment.
Model workflow outcomes as explicit statuses, not booleans.
These fields are reset to false on entry, but the downstream guards in verifying.ts, reviewing.ts, and awaiting-ci.ts also treat false as "completed and failed". That collapses pending and failed into the same value, so the workflow can bounce backward without ever recording the corresponding completion event. Use an explicit status such as pending | passed | failed or track completion separately.
As per coding guidelines: "SD-003: Make illegal states unrepresentable. Use discriminated unions instead of optional fields. If a state combination shouldn't exist, make the type system forbid it."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/states/implementing.ts`
around lines 20 - 24, The fields verifyPassed, reviewPassed, and ciPassed
currently reset to boolean false in implementing.onEntry which conflates
"pending" and "failed"; change these to an explicit status enum or discriminated
union (e.g., "pending" | "passed" | "failed" or a {status: ..., finalized:
boolean} shape) and update the implementing.onEntry initializer to set them to
"pending" (or the equivalent initial union variant); then update the
guards/consumers in verifying.ts, reviewing.ts, and awaiting-ci.ts to check the
new status union instead of treating false as failure/completion so the workflow
can distinguish pending vs failed outcomes.
Owner
Author
There was a problem hiding this comment.
Rejected. passed: boolean is the correct domain model for reviews — they pass or fail, there is no third state. The failedReviewers list captures the detail of which reviewers failed.
There was a problem hiding this comment.
@NTCoding, understood — the state machine structure itself prevents the ambiguity I was concerned about. Since you can only be in a state like VERIFYING after a proper transition, and guards prevent advancing without recording a result, false on entry to IMPLEMENTING is unambiguously a reset rather than a "failed" signal. The domain is binary by design: verify/review/CI either passes or it doesn't, and failedReviewers covers the detail. I'll note this for future reviews.
✏️ Learnings added
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 274
File: tools/dev-workflow-v2/src/workflow-definition/domain/states/implementing.ts:26-31
Timestamp: 2026-03-07T20:51:26.358Z
Learning: In tools/dev-workflow-v2/src/workflow-definition/domain/states/implementing.ts: The boolean flags `verifyPassed`, `reviewPassed`, `ciPassed`, `feedbackClean`, and `feedbackAddressed` are intentionally reset to `false` on entry to the IMPLEMENTING state. The domain model is binary (pass or fail) with no "pending" third state — the state machine's transition guards and required result-recording operations prevent any ambiguity between "not yet run" and "failed". The `failedReviewers` list captures reviewer-level failure detail. Do not suggest replacing these booleans with explicit status enums.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 205
File: tools/dev-workflow/features/complete-task/domain/steps/run-code-review.ts:115-122
Timestamp: 2026-01-21T08:49:10.319Z
Learning: In tools/dev-workflow/features/complete-task/domain/steps/run-code-review.ts: The runtime validation check against validReviewerSet in executeCodeReviewAgents is intentionally kept despite TypeScript's readonly ReviewerName[] type constraint. This defensive coding provides insurance against future refactors that might weaken type constraints.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 186
File: tools/dev-workflow/complete-task/steps/code-review.ts:58-85
Timestamp: 2026-01-17T10:28:11.222Z
Learning: In tools/dev-workflow: Reviewer execution uses Promise.all (fail-fast) rather than Promise.allSettled. If any reviewer fails, the entire workflow should halt immediately to surface the failure. Partial results would hide failures and violate the fail-fast principle.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 0
File: :0-0
Timestamp: 2026-01-30T13:20:59.305Z
Learning: In NTCoding/living-architecture: CodeRabbit has a recurring bug where it submits APPROVED reviews followed by CHANGES_REQUESTED reviews within seconds (e.g., PR `#226`: approved at 13:07:33, changes requested at 13:07:59). The user has `request_changes_workflow: true` correctly configured and wants PRs blocked when changes are needed. The issue is NOT the configuration - it's that CodeRabbit submits premature approvals before completing analysis, then contradicts itself with changes requested. This is a defect in CodeRabbit's review sequencing, not a configuration problem.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 186
File: tools/dev-workflow/complete-task/format-complete-task-result.ts:30-33
Timestamp: 2026-01-17T13:32:27.958Z
Learning: In tools/dev-workflow: The errorDetailsSchema in format-complete-task-result.ts intentionally includes 'done' alongside error types ('fix_errors', 'fix_review', 'resolve_feedback') because it reflects all possible step result types from the workflow runner, not just error types. The schema name is acknowledged as misleading but the behavior is correct.
<!--
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 227
File: .claude/agents/code-review.md:58-78
Timestamp: 2026-01-30T21:29:34.814Z
Learning: In `.claude/agents/code-review.md`, the requirement to audit all 54 rules (SD-001..SD-021, AP-001..AP-006, SP-001..SP-002, TS-001..TS-011, CS-001..CS-008, CR-001..CR-006) with explicit verdicts for every PR is intentional. Previous category-level checkboxes missed violations (e.g., SD-019). N/A rows serve as proof that rules were considered rather than forgotten. The exhaustive approach prevents blind spots even at the cost of verbosity, with the overhead borne by the agent rather than developers.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: docs/conventions/software-design.md:0-0
Timestamp: 2026-02-04T14:15:30.567Z
Learning: Applies to docs/conventions/**/*.{ts,tsx} : SD-003: Make illegal states unrepresentable. Use discriminated unions instead of optional fields. If a state combination shouldn't exist, make the type system forbid it.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 213
File: docs/design-reviews/eclair/critique.md:17-20
Timestamp: 2026-01-28T11:57:43.477Z
Learning: In NTCoding/living-architecture design documents (docs/design-reviews/**/refined.md), "Status: PASS" on checklist items means "the proposed design addresses this principle," not "the implementation exists in the codebase." These are target-state architecture documents showing the intended design, not assessment documents of current code.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 227
File: tools/dev-workflow/platform/domain/workflow-execution/run-workflow.ts:14-23
Timestamp: 2026-01-30T22:55:48.964Z
Learning: In tools/dev-workflow/platform/domain/workflow-execution/run-workflow.ts: Type aliases and interfaces (ContextBuilder, ResultFormatter, WorkflowOptions) used only in exported function signatures should remain unexported per YAGNI principle. TypeScript can inline these in declaration files without explicit exports. Only export if actual TypeScript declaration emit errors occur.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 226
File: tools/dev-workflow/platform/infra/external-clients/github-rest-client.ts:85-118
Timestamp: 2026-01-30T13:52:18.452Z
Learning: In tools/dev-workflow/platform/infra/external-clients/github-rest-client.ts: The extractAssignees, extractLabels, and extractIssueFields functions use manual type guards with flatMap for element-by-element defensive filtering of Octokit API responses. This pattern is intentional for internal (non-exported) helpers that transform loosely-typed API data, allowing individual malformed elements to be filtered out without rejecting entire responses. This is distinct from Zod validation at system boundaries.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: docs/conventions/software-design.md:0-0
Timestamp: 2026-02-04T14:15:30.567Z
Learning: Applies to docs/conventions/**/*.{ts,tsx} : SD-018: Type-Driven Design. Use the type system to encode business rules. Make illegal states unrepresentable using discriminated unions and branded types. Use the type system for validation.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/get-pr-feedback.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/respond-to-feedback.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: CR
Repo: NTCoding/living-architecture PR: 0
File: tools/dev-workflow/CLAUDE.md:0-0
Timestamp: 2026-02-06T22:44:38.511Z
Learning: Applies to tools/dev-workflow/**/complete-task.ts : Entry points should import `runWorkflow` from shared infrastructure, import context builder from the same command directory, and declare steps on separate lines for readability.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 186
File: tools/dev-workflow/get-pr-feedback/steps/fetch-feedback.ts:7-20
Timestamp: 2026-01-17T13:19:34.131Z
Learning: In tools/dev-workflow: command-specific types (e.g., PRStatus in fetch-feedback.ts) should remain internal to their command directory and not be exported. This enforces the architecture principle that types scoped to a specific command/workflow step should not be shared outside that command's directory.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 120
File: docs/workflow/parallel-workflow.md:160-281
Timestamp: 2026-01-10T19:16:54.359Z
Learning: In the NTCoding/living-architecture repository, design and investigation documents (such as those in docs/workflow/) can reference scripts, commands, or features that don't yet exist in the codebase. These references serve as specifications for future implementation tasks, not documentation of current capabilities. This is acceptable when the PR scope is research/design rather than implementation.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 205
File: tools/dev-workflow/vitest.config.mts:18-21
Timestamp: 2026-01-21T08:49:07.503Z
Learning: In tools/dev-workflow, coverage enforcement is intentionally scoped to only tested modules (e.g., features/claude-hooks/domain/handlers, features/complete-task/domain/task-check-marker.ts). The majority of platform/ and features/ code is tested through integration testing of the complete-task pipeline with manual verification. Unit tests for thin wrapper code in this package are considered low-value relative to maintenance cost.
Learnt from: NTCoding
Repo: NTCoding/living-architecture PR: 129
File: apps/eclair/src/components/SchemaModal/SchemaModal.cssModule.spec.ts:13-19
Timestamp: 2026-01-11T18:47:09.232Z
Learning: Encourage using const for declarations by default in TypeScript/JavaScript files and only use let when a variable must be reassigned. Do not rely on var. This improves immutability and clarity. If a variable needs mutation (e.g., counters in loops, reassigned values), use let, but prefer const whenever possible and refactor reassigned logic to avoid mutations where feasible.
Comment on lines
+42
to
+43
| static createFresh(deps: WorkflowDeps): Workflow { | ||
| return new Workflow(EMPTY_STATE, deps) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd tools/dev-workflow-v2 && find . -name "workflow.ts" | head -5Repository: NTCoding/living-architecture
Length of output: 116
🏁 Script executed:
find . -name "workflow.ts" -path "*/workflow-definition/domain/*" | head -1Repository: NTCoding/living-architecture
Length of output: 138
🏁 Script executed:
cat -n tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts | head -100Repository: NTCoding/living-architecture
Length of output: 3368
🏁 Script executed:
cat -n tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts | sed -n '100,200p'Repository: NTCoding/living-architecture
Length of output: 3449
🏁 Script executed:
cat -n tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts | tail -100Repository: NTCoding/living-architecture
Length of output: 3519
🏁 Script executed:
grep -n "EMPTY_STATE" tools/dev-workflow-v2/src/workflow-definition/domain/*.tsRepository: NTCoding/living-architecture
Length of output: 4083
🏁 Script executed:
cat -n tools/dev-workflow-v2/src/workflow-definition/domain/fold.ts | head -50Repository: NTCoding/living-architecture
Length of output: 1683
🏁 Script executed:
cat -n tools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.ts | head -80Repository: NTCoding/living-architecture
Length of output: 2747
🏁 Script executed:
cd tools/dev-workflow-v2 && rg "getState\(\)" -A 3 -B 1 --type tsRepository: NTCoding/living-architecture
Length of output: 4704
🏁 Script executed:
cd tools/dev-workflow-v2 && rg "\.state\s*=" -A 2 --type ts src/workflow-definition/domain/workflow.tsRepository: NTCoding/living-architecture
Length of output: 238
Do not expose the live workflow state object.
createFresh() passes the shared EMPTY_STATE directly to the constructor, and getState() returns the mutable internal state reference unchanged. Callers can mutate state without emitting an event, violating the event-sourced invariants.
🧱 Suggested hardening
static createFresh(deps: WorkflowDeps): Workflow {
- return new Workflow(EMPTY_STATE, deps)
+ return new Workflow({ ...EMPTY_STATE }, deps)
}
getState(): WorkflowState {
- return this.state
+ return { ...this.state }
}Also applies to: 63-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts` around
lines 42 - 43, createFresh currently passes the shared mutable EMPTY_STATE into
the Workflow constructor and getState returns the internal state directly,
exposing mutable references; change createFresh (and the Workflow constructor if
it directly assigns) to pass a defensive copy of EMPTY_STATE (deep clone or
Object.freeze a newly created object) and update getState to return a
deep/shallow copy (or a frozen snapshot) instead of the internal state reference
so callers cannot mutate the workflow state without raising events; refer to
createFresh, Workflow constructor, and getState to implement these defensive
copies.
Owner
Author
There was a problem hiding this comment.
Rejected. Event-sourced state is rebuilt from events. Mutation without going through event recording breaks rehydration. structuredClone overhead is not warranted for an internal API.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 11 seconds before sending another message.
Comment on lines
+243
to
+245
| transitionTo(target: string): PreconditionResult { | ||
| const from = parseStateName(this.state.currentStateMachineState) | ||
| const targetState = parseStateName(target) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd tools/dev-workflow-v2 && find . -name "workflow.ts" -type fRepository: NTCoding/living-architecture
Length of output: 116
🏁 Script executed:
cd tools/dev-workflow-v2 && head -n 300 src/workflow-definition/domain/workflow.ts | tail -n 100Repository: NTCoding/living-architecture
Length of output: 2819
🏁 Script executed:
cd tools/dev-workflow-v2 && sed -n '240,260p' src/workflow-definition/domain/workflow.tsRepository: NTCoding/living-architecture
Length of output: 843
🏁 Script executed:
cd tools/dev-workflow-v2 && rg "parseStateName" -A 5 -B 2Repository: NTCoding/living-architecture
Length of output: 4522
🏁 Script executed:
cd tools/dev-workflow-v2 && rg "STATE_NAME_SCHEMA" -B 2 -A 5Repository: NTCoding/living-architecture
Length of output: 3272
Use safeParse() to handle invalid target state names instead of throwing.
The transitionTo() method should validate the target input using STATE_NAME_SCHEMA.safeParse() instead of calling parseStateName(target), which uses .parse() and throws on invalid state names. Since the method already returns PreconditionResult and models failures with fail(), invalid targets should be converted to a failed precondition result rather than crashing the caller.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts` around
lines 243 - 245, Replace the direct parse call in transitionTo to validate the
target with STATE_NAME_SCHEMA.safeParse() instead of parseStateName(target); if
safeParse() fails, return a failed PreconditionResult via fail() with a clear
message about the invalid target, otherwise extract the parsed value and
continue using it (keep the existing
parseStateName(this.state.currentStateMachineState) usage for the current
state). Ensure you reference the parsed target value (from safeParse().data)
where parseStateName(target) was used and that any error path returns fail(...)
rather than throwing.
Owner
Author
There was a problem hiding this comment.
Obsolete. transitionTo() was deleted — the platform handles transitions now.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 6 seconds before sending another message.
Comment on lines
+1
to
+19
| { | ||
| "compilerOptions": { | ||
| "target": "ES2022", | ||
| "module": "ESNext", | ||
| "moduleResolution": "bundler", | ||
| "strict": true, | ||
| "noUncheckedIndexedAccess": true, | ||
| "exactOptionalPropertyTypes": true, | ||
| "verbatimModuleSyntax": true, | ||
| "outDir": "dist", | ||
| "rootDir": "src", | ||
| "esModuleInterop": true, | ||
| "skipLibCheck": true, | ||
| "resolveJsonModule": true, | ||
| "types": ["vitest/globals", "node"] | ||
| }, | ||
| "include": ["src/**/*"], | ||
| "exclude": ["node_modules", "dist"] | ||
| } |
There was a problem hiding this comment.
Missing required 3-file tsconfig structure.
This new project has only a single tsconfig.json. Per coding guidelines, new projects require a 3-file structure: tsconfig.json, tsconfig.lib.json, and tsconfig.spec.json.
As per coding guidelines: "Create 3-file tsconfig structure (tsconfig.json, tsconfig.lib.json, tsconfig.spec.json) for new projects"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/tsconfig.json` around lines 1 - 19, Replace the single
tsconfig.json with the required 3-file structure: keep a minimal root
tsconfig.json that sets "files": [], "references" or simply extends
per-guideline and delegates to tsconfig.lib.json and tsconfig.spec.json; add
tsconfig.lib.json containing the build-facing compilerOptions
(target/module/moduleResolution/outDir/rootDir, declaration true if library, no
tests, include "src/**/*", exclude "dist","**/*.spec.ts") and ensure it
preserves strict/noUncheckedIndexedAccess/exactOptionalPropertyTypes; add
tsconfig.spec.json for tests with test-friendly compilerOptions (include types
like "vitest/globals","node", include test files pattern such as
"src/**/*.spec.ts" and any mocks) and make sure "skipLibCheck" and
"resolveJsonModule" match project needs; update any "types" or
"include"/"exclude" entries from the original tsconfig.json into the appropriate
lib/spec files so the build and tests resolve correctly.
🛠️ Refactor suggestion | 🟠 Major
Consider extending tsconfig.base.json to align with monorepo standards.
This config does not extend tsconfig.base.json, causing divergence from monorepo-wide TypeScript contracts. Missing flags include:
composite,declarationMap,isolatedModules,importHelpersnoImplicitOverride,noImplicitReturns,noUnusedLocals,noUnusedParameters,noPropertyAccessFromIndexSignaturelib/targetuses ES2022 vs es2023 in basecustomConditionsfor@living-architecture/source
♻️ Suggested structure extending base
{
+ "extends": "../../tsconfig.base.json",
"compilerOptions": {
- "target": "ES2022",
- "module": "ESNext",
- "moduleResolution": "bundler",
- "strict": true,
- "noUncheckedIndexedAccess": true,
- "exactOptionalPropertyTypes": true,
- "verbatimModuleSyntax": true,
"outDir": "dist",
"rootDir": "src",
- "esModuleInterop": true,
- "skipLibCheck": true,
- "resolveJsonModule": true,
"types": ["vitest/globals", "node"]
},
"include": ["src/**/*"],
"exclude": ["node_modules", "dist"]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/tsconfig.json` around lines 1 - 19, Update
tools/dev-workflow-v2/tsconfig.json to extend tsconfig.base.json and
merge/override only needed options: add "extends": "tsconfig.base.json" at the
top, then ensure compilerOptions includes the missing build flags (composite,
declarationMap, isolatedModules, importHelpers), the stricter checks
(noImplicitOverride, noImplicitReturns, noUnusedLocals, noUnusedParameters,
noPropertyAccessFromIndexSignature), align lib/target with the base (use ES2023
or inherit), and add customConditions for "@living-architecture/source"; keep
existing options (module, moduleResolution, types, etc.) but prefer inheriting
common settings from the base so the package follows monorepo standards.
Owner
Author
There was a problem hiding this comment.
Rejected. tools/ projects are Claude plugins, not publishable NX packages. They don't need the 3-file tsconfig structure that published packages require.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 9 seconds before sending another message.
| "verbatimModuleSyntax": true, | ||
| "outDir": "dist", | ||
| "rootDir": "src", | ||
| "esModuleInterop": true, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
esModuleInterop is redundant with verbatimModuleSyntax.
When verbatimModuleSyntax is enabled (line 9), it enforces explicit import/export syntax and makes esModuleInterop unnecessary. The base config omits esModuleInterop for this reason.
♻️ Proposed fix
"outDir": "dist",
"rootDir": "src",
- "esModuleInterop": true,
"skipLibCheck": true,📝 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
| "esModuleInterop": true, | |
| "outDir": "dist", | |
| "rootDir": "src", | |
| "skipLibCheck": true, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/tsconfig.json` at line 12, Remove the redundant
"esModuleInterop" compiler option since "verbatimModuleSyntax" is enabled in
this tsconfig; locate the options block containing verbatimModuleSyntax and
esModuleInterop and delete the esModuleInterop entry so the config relies on
verbatimModuleSyntax as in the base config, and run a quick typecheck to confirm
no downstream code depends on esModuleInterop semantics.
Owner
Author
There was a problem hiding this comment.
Rejected. Removing esModuleInterop changes behavior for default imports from CJS modules (e.g., import path from 'node:path'). Keeping it for safety.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 3 seconds before sending another message.
…re and CLI entrypoint Event store: SQLite persistence with better-sqlite3 for event sourcing. CLI entrypoint: 15 command handlers + hook routing (SessionStart, PreToolUse). Infra layer: composition root, environment config, git adapter, hook I/O parsing. 253 tests at 100% coverage across all metrics. Co-Authored-By: Claude Opus 4.6 <[email protected]>
… and API - Remove resolveStringField (AP-004 violation) — fail fast for missing Bash command - Move workflow-error.ts to domain/ (not infra) - Move environment.ts, stdin.ts, git.ts, hook-io.ts to infra/cli/ - Move composition-root.ts to shell/ - Move sqlite-event-store to infra/persistence/ - Move test fixtures to entrypoint/fixtures/ - Delete invalid infra/domain/ directory Co-Authored-By: Claude Opus 4.6 <[email protected]>
Deploying living-architecture with
|
| Latest commit: |
1eaa36c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d3ccce50.living-architecture.pages.dev |
| Branch Preview URL: | https://worktree-composed-splashing.living-architecture.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 23
♻️ Duplicate comments (2)
tools/dev-workflow-v2/package.json (2)
11-11:⚠️ Potential issue | 🟠 MajorPin the GitHub dependency to an immutable ref.
Line 11 tracks the repository default branch, so a lockfile refresh can pull a different
@ntcoding/agentic-workflow-builderrevision without any manifest change.Suggested fix
- "@ntcoding/agentic-workflow-builder": "github:NTCoding/autonomous-claude-agent-team#path:packages/agentic-workflow-builder", + "@ntcoding/agentic-workflow-builder": "github:NTCoding/autonomous-claude-agent-team#<tag-or-sha>&path:packages/agentic-workflow-builder",pnpm GitHub dependency syntax pin tag or commit SHA with subdirectory path in package.json🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/package.json` at line 11, The package.json entry for "@ntcoding/agentic-workflow-builder" is using a floating GitHub ref; update that dependency to an immutable ref by replacing "github:NTCoding/autonomous-claude-agent-team#path:packages/agentic-workflow-builder" with a pinned ref such as a commit SHA or a specific tag and keep the subdirectory path (e.g., "github:NTCoding/autonomous-claude-agent-team#v1.2.3:path:packages/agentic-workflow-builder" or use the commit SHA instead of v1.2.3) so the manifest no longer tracks the default branch.
2-2:⚠️ Potential issue | 🟠 MajorUse the repository package scope for this workspace.
Line 2 declares the new project as an unscoped package, which breaks the monorepo naming convention for new packages.
Suggested fix
- "name": "dev-workflow-v2", + "name": "@living-architecture/dev-workflow-v2",As per coding guidelines, "Update package.json with correct
@living-architecturescoped name when creating new projects".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/package.json` at line 2, Update the package name to use the repository scope: change the "name" field in package.json from the unscoped "dev-workflow-v2" to the scoped package name "@living-architecture/dev-workflow-v2" so it conforms to the monorepo naming convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.mjs`:
- Around line 52-58: The override in eslint.config.mjs is masking a real
shell-injection risk in the runGit function in
tools/dev-workflow-v2/src/infra/cli/git.ts: replace the current execSync call
that interpolates a free-form gitArgs string with an argv-based invocation
(e.g., use execFileSync or spawnSync and pass args as an array) or restrict
gitArgs to a validated closed set of subcommands; update runGit to construct and
pass an array of arguments to the safer API, validate or sanitize any
user-provided parts, and remove the need to disable sonarjs/os-command and
sonarjs/no-os-command-from-path so the root rules can remain enabled.
In `@tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts`:
- Around line 69-133: The function progressToState silently no-ops on unknown
targets and ignores non-zero exit codes; hoist the stateSteps table into a const
(e.g., const stateSteps = {...}) outside progressToState, change the function
signature to progressToState(deps: AdapterDeps, targetState: keyof typeof
stateSteps) to type-check targets, and after calling runWorkflow(['init'], deps)
and after each runWorkflow(step, deps) check the returned exit code and throw an
Error (or rethrow) immediately if it is non-zero; also throw immediately if
targetState is not present (defensive check) so failures fail fast.
- Around line 22-24: buildTestDeps currently creates the SQLite store with
createStore(dbPath) but only returns {deps, dbPath}, so tests cannot close the
DB; modify buildTestDeps to also return the created store (the SqliteEventStore
instance) so callers can call store.db.close() before cleanupDb(dbPath) in
afterEach; update any callers in tests to destructure {deps, dbPath, store} and
invoke await store.db.close() (or .close()) prior to removing files to avoid
leaked descriptors and Windows file-lock failures.
In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.ts`:
- Around line 65-69: The tests only assert exitCode; update each test (using
setup() and runWorkflow(..., deps)) to assert the actual recorded effect: after
runWorkflow(['record-issue','42'], deps) assert the event store or state (e.g.,
deps.eventStore or deps.stateMachine) contains an IssueRecorded event with
number 42; for runWorkflow(['record-verify-passed'], deps) assert the workflow
transitioned to REVIEWING; for runWorkflow(['record-pr', ...], deps) assert
transition to AWAITING_CI; for runWorkflow(['record-reflection', ...], deps)
assert transition to COMPLETE — locate and use the helpers in the spec (setup,
runWorkflow) and the injected deps (event store / state machine) to make one
explicit observable assertion per test instead of only checking exitCode.
- Around line 155-167: Tests call runWorkflow(['init'], deps) then run record-*
commands, which can fail due to state gating instead of argument validation;
update each test to first advance the workflow into the correct state (use
setup() + runWorkflow calls to reach SUBMITTING_PR before testing record-pr,
CHECKING_FEEDBACK before record-feedback-exists, and REFLECTING before
record-reflection), then call runWorkflow(['record-pr', ...]) (or the
corresponding record-* command) and assert the specific validation
message/stdout (not just exitCode) for missing or non-numeric PR number; apply
the same pattern to the other test blocks mentioned (lines ~212-224 and
~244-248) so assertions exercise argument validation rather than state
rejection.
In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.ts`:
- Around line 69-73: The catch block currently calls
appendFileSync(getErrorLogPath(), message) which can throw (e.g., missing
~/.claude/ dir) and mask the original error; update the catch handler (the catch
block surrounding appendFileSync in workflow-cli.ts) to perform the file write
inside its own try-catch: before writing ensure the directory exists (use
mkdirSync(pathDir, { recursive: true }) on the directory portion of
getErrorLogPath()), then call appendFileSync, and if any filesystem operation
fails, log that filesystem error to process.stderr (but do not overwrite the
original message) so the original error remains visible; finally still call
process.exit(EXIT_ERROR).
In `@tools/dev-workflow-v2/src/infra/cli/command-handlers.ts`:
- Around line 62-68: The CLI currently uses Number.parseInt(rawNumber, 10) and
Number.isNaN to accept malformed values like "12abc", "0x10", "0", or negative
numbers; update the three handlers (record-issue, record-pr,
record-feedback-exists) to first validate rawNumber with a strict
positive-integer check (e.g., match against /^[1-9]\d*$/) before parsing, and if
it fails return the same error shape (output: `... not a valid number:
'${rawNumber}'`, exitCode: EXIT_ERROR); then safely parse the validated token
(Number.parseInt) to get the numeric id/count and proceed as before.
In `@tools/dev-workflow-v2/src/infra/cli/environment.ts`:
- Around line 4-25: The helpers (getSessionId, getPluginRoot, getEnvFilePath)
currently only reject undefined, allowing empty or whitespace env values to slip
through; update each function to read the raw env var, check if it is undefined
OR when trimmed is an empty string, and if so throw WorkflowError with the same
missing-env message; otherwise return the trimmed value (use the function names
getSessionId, getPluginRoot, getEnvFilePath and keep throwing WorkflowError) so
callers receive validated, sanitized strings at the boundary.
In `@tools/dev-workflow-v2/src/infra/cli/git.ts`:
- Around line 18-23: detectDefaultBranch currently swallows errors and returns a
hardcoded 'main'; instead, after obtaining the candidate branch from
runGit('symbolic-ref ...') (or catching the failure), verify that the candidate
actually exists in the repo (e.g., via a git ref/verify or rev-parse check) and
only return it if the verification succeeds; if no candidate can be verified (no
origin HEAD or neither main/master/trunk exist), throw a WorkflowError with a
clear message that default-branch detection failed and list the branches checked
so callers can act accordingly; update detectDefaultBranch and use the existing
runGit helper and WorkflowError class names referenced in the file.
- Around line 1-2: The code uses execSync with shell-interpolated git commands
(see execSync usage in git.ts) embedding user-controlled values like
defaultBranch (returned by detectDefaultBranch()) into command strings, which
allows shell injection; replace execSync calls with execFileSync (or spawnSync)
and pass the git binary as the first argument and the git subcommand and branch
names as elements of an argv array (e.g., ['rev-parse', '--abbrev-ref',
defaultBranch] or ['show-ref', '--verify', `refs/heads/${defaultBranch}`]) so
arguments are not shell-interpolated, and validate or normalize defaultBranch
where appropriate before passing it to execFileSync.
In `@tools/dev-workflow-v2/src/infra/cli/hook-handlers.ts`:
- Around line 82-98: The handler currently treats only hookCheck.type ===
'blocked' as denial, causing any 'error' result from
engine.transaction(hookInput.session_id, 'hook-check', (w) =>
w.checkBashAllowed(hookInput.tool_name, command), hookInput.transcript_path) to
fall through to EXIT_ALLOW; change the logic to treat hookCheck.type === 'error'
as a failure (fail-closed) — return an object with output set to a formatted
error/deny message (use formatDenyDecision or include hookCheck.output) and
exitCode set to EXIT_BLOCK, ensuring both 'blocked' and 'error' paths deny
execution instead of allowing it.
In `@tools/dev-workflow-v2/src/infra/cli/hook-io.ts`:
- Around line 4-16: HOOK_COMMON_INPUT currently allows empty/whitespace-only
strings because it uses z.string(); tighten the schema by requiring non-empty,
trimmed strings for identifiers/paths (session_id, transcript_path, cwd,
hook_event_name and optional permission_mode) and also enforce tool_name and
tool_use_id in PRE_TOOL_USE_INPUT to be normalized non-empty strings; update
HOOK_COMMON_INPUT and PRE_TOOL_USE_INPUT to trim inputs and validate min length
(or use a refinement) so values like " Bash " or "" are rejected before
routing/filtering and ensure tool_input remains a record of unknowns.
In `@tools/dev-workflow-v2/src/infra/persistence/sqlite-event-store.spec.ts`:
- Around line 37-57: The test currently only asserts count and event.type;
update the assertions to verify the full persisted event objects are
round-tripped (including at, from, to and any payload fields) by comparing the
returned array from store.readEvents(...) to the original events array (e.g.
expect(retrieved).toStrictEqual(events) or map/normalize if store adds
metadata), and make the same change in the other similar test (lines 72-93);
locate uses of freshStore(), store.appendEvents('sess-1', ...),
store.readEvents('sess-1') and store.db.close() to implement this replacement.
In `@tools/dev-workflow-v2/src/infra/persistence/sqlite-event-store.ts`:
- Around line 23-29: The public SqliteEventStore type currently exposes the raw
Database via the db property which allows callers to bypass
appendEvents/readEvents and break the event-store boundary; remove the db field
from the exported SqliteEventStore type and instead expose only the controlled
API (readEvents, appendEvents, sessionExists, listSessions) and, if necessary,
add a close() method to the type for lifecycle management; keep access to the
underlying Database handle confined to internal/module scope or provide a
separate test-only helper that returns the Database for tests (do not expose
Database in the public type or signatures).
In `@tools/dev-workflow-v2/src/shell/composition-root.ts`:
- Around line 33-37: The current real dependency wiring sets
WorkflowDeps.checkPrChecks to a noop (() => true) in the workflowDeps const,
which effectively disables PR/CI gating; update
buildRealDeps/composition-root.ts so workflowDeps.checkPrChecks is wired to the
real implementation (or fails fast) instead of returning true—locate the
workflowDeps const and replace checkPrChecks: () => true with a call to the real
PR-checks adapter/function (or throw an error if that adapter is not available),
ensuring WorkflowDeps.checkPrChecks returns actual CI status or fails fast when
the adapter is missing.
In `@tools/dev-workflow-v2/states/addressing-feedback.md`:
- Around line 8-13: The checklist forces "Commit all fixes" even when a thread
is rejected with no code changes; update the workflow so the commit step runs
only if there are actual repo changes: detect modifications (e.g., via git diff
--quiet / git status --porcelain) before executing the "Commit all fixes" action
referenced by the checklist item and the responding command flow (pnpm nx run
dev-workflow:respond-to-feedback and /dev-workflow-v2:workflow
record-feedback-addressed); if no changes are detected, skip the commit step and
proceed to record the feedback as addressed.
In `@tools/dev-workflow-v2/states/awaiting-ci.md`:
- Around line 9-12: Make the two-step dependency explicit: state that the record
commands (/dev-workflow-v2:workflow record-ci-passed and
/dev-workflow-v2:workflow record-ci-failed) must be executed before the
transition commands (/dev-workflow-v2:workflow transition CHECKING_FEEDBACK and
/dev-workflow-v2:workflow transition IMPLEMENTING) because the record actions
set ciPassed which the transition constraints require; update the TODO lines to
show the order (record first, then transition) and call out the ciPassed
requirement so the sequence is unambiguous.
- Line 18: The AWAITING_CI state's transition to BLOCKED is underspecified;
update the line that currently reads "If blocked, transition to BLOCKED:
`/dev-workflow-v2:workflow transition BLOCKED`" (AWAITING_CI) to add brief
clarifying conditions or a TODO list describing when to use BLOCKED (e.g., CI
stuck/stalled, external dependency unavailable, PR closed/abandoned, or other
manual-intervention cases) so reviewers/operators know when to run the
transition command.
In `@tools/dev-workflow-v2/states/complete.md`:
- Around line 3-9: Update the final notification in the complete.md TODO so it’s
conditional on the PR merge status: change the "Notify the user the PR is ready
for their review" checklist item (the final notification step) to either emit a
generic completion message or split it into two outcomes (one message for "PR
merged" and a different "PR ready for review" message) and ensure the wording
depends on PR status rather than always saying "ready for review."
In `@tools/dev-workflow-v2/states/reviewing.md`:
- Around line 11-14: The REVIEWING workflow currently instructs to fix/commit
first then run "/dev-workflow-v2:workflow record-review-failed", which loses the
snapshot of the failed review; change the sequence so that when a review fails
you first run "/dev-workflow-v2:workflow record-review-failed", then run
"/dev-workflow-v2:workflow transition IMPLEMENTING", and only after that perform
corrective commits in IMPLEMENTING; update the checklist items referencing
"record-review-failed" and "transition IMPLEMENTING" to reflect this ordering
and ensure the passing path still records the pass before transitioning to
"SUBMITTING_PR" via "/dev-workflow-v2:workflow record-review-passed" then
"/dev-workflow-v2:workflow transition SUBMITTING_PR".
In `@tools/dev-workflow-v2/states/submitting-pr.md`:
- Around line 8-15: Update the user-facing checklist and constraints to be
command-accurate: change the "Create the PR" line to show both create and update
flows (e.g., keep the existing example `gh pr create --title "<title>" --body
"<description>"` and add an explicit update example or note such as using `gh pr
edit <PR_NUMBER> --title ... --body ...`), clarify the allowed commands entry so
it only exempts the specific commands used in this workflow (reference the
existing lines mentioning `git push` and `gh pr`) rather than implying all
variants are allowed, and replace the internal field name `prNumber` with
user-facing wording like "PR number" in the constraint about transitioning to
AWAITING_CI (reference the `Record the PR: /dev-workflow-v2:workflow record-pr
<PR_NUMBER> [PR_URL]` and `Transition to AWAITING_CI: /dev-workflow-v2:workflow
transition AWAITING_CI` lines).
In `@tools/dev-workflow-v2/states/verifying.md`:
- Around line 7-11: Update the VERIFYING workflow steps so a failed verification
is recorded before any code changes: when `pnpm verify` fails first call
`/dev-workflow-v2:workflow record-verify-failed`, then call
`/dev-workflow-v2:workflow transition IMPLEMENTING`, and only after those two
actions proceed to fix the errors, commit, and re-run `pnpm verify`; adjust the
lines referencing `/dev-workflow-v2:workflow record-verify-failed` and the
transition order accordingly.
In `@tools/dev-workflow-v2/vitest.config.mts`:
- Around line 3-21: The Vitest config lacks a root setting so coverage paths
resolve from the process CWD; add root: __dirname at the top-level of the
exported config (the object passed to defineConfig) so test.coverage.include and
test.coverage.reportsDirectory are resolved relative to this package; update the
object returned by defineConfig (the export default) to include root: __dirname
alongside the existing test and coverage properties.
---
Duplicate comments:
In `@tools/dev-workflow-v2/package.json`:
- Line 11: The package.json entry for "@ntcoding/agentic-workflow-builder" is
using a floating GitHub ref; update that dependency to an immutable ref by
replacing
"github:NTCoding/autonomous-claude-agent-team#path:packages/agentic-workflow-builder"
with a pinned ref such as a commit SHA or a specific tag and keep the
subdirectory path (e.g.,
"github:NTCoding/autonomous-claude-agent-team#v1.2.3:path:packages/agentic-workflow-builder"
or use the commit SHA instead of v1.2.3) so the manifest no longer tracks the
default branch.
- Line 2: Update the package name to use the repository scope: change the "name"
field in package.json from the unscoped "dev-workflow-v2" to the scoped package
name "@living-architecture/dev-workflow-v2" so it conforms to the monorepo
naming convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e3f97e2-b612-4c3d-b326-ed5f4e444717
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
eslint.config.mjstools/dev-workflow-v2/package.jsontools/dev-workflow-v2/src/domain/workflow-error.spec.tstools/dev-workflow-v2/src/domain/workflow-error.tstools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.tstools/dev-workflow-v2/src/entrypoint/workflow-cli-hooks.spec.tstools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.tstools/dev-workflow-v2/src/entrypoint/workflow-cli.tstools/dev-workflow-v2/src/infra/cli/command-handlers.tstools/dev-workflow-v2/src/infra/cli/environment.spec.tstools/dev-workflow-v2/src/infra/cli/environment.tstools/dev-workflow-v2/src/infra/cli/git.tstools/dev-workflow-v2/src/infra/cli/hook-handlers.tstools/dev-workflow-v2/src/infra/cli/hook-io.spec.tstools/dev-workflow-v2/src/infra/cli/hook-io.tstools/dev-workflow-v2/src/infra/cli/operation-result.spec.tstools/dev-workflow-v2/src/infra/cli/operation-result.tstools/dev-workflow-v2/src/infra/cli/stdin.tstools/dev-workflow-v2/src/infra/persistence/sqlite-event-store.spec.tstools/dev-workflow-v2/src/infra/persistence/sqlite-event-store.tstools/dev-workflow-v2/src/shell/composition-root.tstools/dev-workflow-v2/states/addressing-feedback.mdtools/dev-workflow-v2/states/awaiting-ci.mdtools/dev-workflow-v2/states/blocked.mdtools/dev-workflow-v2/states/checking-feedback.mdtools/dev-workflow-v2/states/complete.mdtools/dev-workflow-v2/states/implementing.mdtools/dev-workflow-v2/states/reflecting.mdtools/dev-workflow-v2/states/reviewing.mdtools/dev-workflow-v2/states/submitting-pr.mdtools/dev-workflow-v2/states/verifying.mdtools/dev-workflow-v2/vitest.config.mts
Comment on lines
+52
to
+58
| { | ||
| files: ['**/infra/cli/git.ts'], | ||
| rules: { | ||
| 'sonarjs/os-command': 'off', | ||
| 'sonarjs/no-os-command-from-path': 'off', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Do not weaken the root lint policy to allow shell interpolation here.
This override masks a real problem in tools/dev-workflow-v2/src/infra/cli/git.ts:26-28, where execSync(\git ${gitArgs}`)builds a shell command from a free-form string. Keeping these Sonar rules off ineslint.config.mjsweakens a repo-wide guardrail, and the**/infra/cli/git.tsglob will also silence future files that match the pattern. FixrunGit to use argv-based execution (execFileSync/spawnSync`) or a closed set of subcommands, then leave the rules enabled.
🛠️ Proposed change in this file
- {
- files: ['**/infra/cli/git.ts'],
- rules: {
- 'sonarjs/os-command': 'off',
- 'sonarjs/no-os-command-from-path': 'off',
- },
- },Based on learnings: Never modify root configuration files (eslint.config.mjs, tsconfig.base.json, nx.json, vite.config, vitest.config.mts) without user approval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.mjs` around lines 52 - 58, The override in eslint.config.mjs is
masking a real shell-injection risk in the runGit function in
tools/dev-workflow-v2/src/infra/cli/git.ts: replace the current execSync call
that interpolates a free-form gitArgs string with an argv-based invocation
(e.g., use execFileSync or spawnSync and pass args as an array) or restrict
gitArgs to a validated closed set of subcommands; update runGit to construct and
pass an array of arguments to the safer API, validate or sanitize any
user-provided parts, and remove the need to disable sonarjs/os-command and
sonarjs/no-os-command-from-path so the root rules can remain enabled.
Owner
Author
There was a problem hiding this comment.
Rejected. Root config file — CLAUDE.md prohibits modification. The underlying shell injection issue in git.ts has been fixed directly (switched to execFileSync with argument arrays), making this override moot.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 7 seconds before sending another message.
Comment on lines
+22
to
+24
| const tempDir = mkdtempSync(join(tmpdir(), 'wf-cli-')) | ||
| const dbPath = join(tempDir, 'test.db') | ||
| const store = createStore(dbPath) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tools/dev-workflow-v2/src/infra/persistence/sqlite-event-store.ts =="
fd 'sqlite-event-store\.ts$' tools/dev-workflow-v2/src -x sed -n '1,220p' {}
echo
echo "== database lifecycle signals under tools/dev-workflow-v2/src =="
rg -n -C3 '\b(better-sqlite3|new Database|Database\(|\.close\(|dispose)\b' tools/dev-workflow-v2/srcRepository: NTCoding/living-architecture
Length of output: 4307
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full test fixture file =="
cat -n tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts
echo
echo "== Check for close() calls in entire dev-workflow-v2 =="
rg '\.close\(\)' tools/dev-workflow-v2/srcRepository: NTCoding/living-architecture
Length of output: 6123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Test files using buildTestDeps/cleanupDb =="
rg -n 'buildTestDeps|cleanupDb' tools/dev-workflow-v2 --type ts -B2 -A2
echo
echo "== afterEach/teardown patterns =="
rg -n 'afterEach|teardown|cleanup' tools/dev-workflow-v2 --type ts -B1 -A3 | head -50Repository: NTCoding/living-architecture
Length of output: 7938
Close the SQLite store before deleting its files.
buildTestDeps() creates a SQLite-backed store via createStore(), but cleanupDb() only unlinks files by pathname without closing the database first. This leaks file descriptors on Unix and breaks cleanup on Windows since the database file remains open.
The SqliteEventStore already exposes the database handle (store.db), but buildTestDeps() does not return it—only the deps and dbPath. Update buildTestDeps() to return the store so afterEach can call store.db.close() before cleanupDb().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts`
around lines 22 - 24, buildTestDeps currently creates the SQLite store with
createStore(dbPath) but only returns {deps, dbPath}, so tests cannot close the
DB; modify buildTestDeps to also return the created store (the SqliteEventStore
instance) so callers can call store.db.close() before cleanupDb(dbPath) in
afterEach; update any callers in tests to destructure {deps, dbPath, store} and
invoke await store.db.close() (or .close()) prior to removing files to avoid
leaked descriptors and Windows file-lock failures.
Owner
Author
There was a problem hiding this comment.
Accepted and fixed. Added closeStore to TestContext. cleanupDb now closes the SQLite connection before deleting files.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 5 seconds before sending another message.
Comment on lines
+69
to
+133
| export function progressToState(deps: AdapterDeps, targetState: string): void { | ||
| const stateSteps: Readonly<Record<string, readonly (readonly string[])[]>> = { | ||
| VERIFYING: [['transition', 'VERIFYING']], | ||
| REVIEWING: [['transition', 'VERIFYING'], ['record-verify-passed'], ['transition', 'REVIEWING']], | ||
| SUBMITTING_PR: [ | ||
| ['transition', 'VERIFYING'], | ||
| ['record-verify-passed'], | ||
| ['transition', 'REVIEWING'], | ||
| ['record-review-passed'], | ||
| ['transition', 'SUBMITTING_PR'], | ||
| ], | ||
| AWAITING_CI: [ | ||
| ['transition', 'VERIFYING'], | ||
| ['record-verify-passed'], | ||
| ['transition', 'REVIEWING'], | ||
| ['record-review-passed'], | ||
| ['transition', 'SUBMITTING_PR'], | ||
| ['record-pr', '1'], | ||
| ['transition', 'AWAITING_CI'], | ||
| ], | ||
| CHECKING_FEEDBACK: [ | ||
| ['transition', 'VERIFYING'], | ||
| ['record-verify-passed'], | ||
| ['transition', 'REVIEWING'], | ||
| ['record-review-passed'], | ||
| ['transition', 'SUBMITTING_PR'], | ||
| ['record-pr', '1'], | ||
| ['transition', 'AWAITING_CI'], | ||
| ['record-ci-passed'], | ||
| ['transition', 'CHECKING_FEEDBACK'], | ||
| ], | ||
| ADDRESSING_FEEDBACK: [ | ||
| ['transition', 'VERIFYING'], | ||
| ['record-verify-passed'], | ||
| ['transition', 'REVIEWING'], | ||
| ['record-review-passed'], | ||
| ['transition', 'SUBMITTING_PR'], | ||
| ['record-pr', '1'], | ||
| ['transition', 'AWAITING_CI'], | ||
| ['record-ci-passed'], | ||
| ['transition', 'CHECKING_FEEDBACK'], | ||
| ['record-feedback-exists', '2'], | ||
| ['transition', 'ADDRESSING_FEEDBACK'], | ||
| ], | ||
| REFLECTING: [ | ||
| ['transition', 'VERIFYING'], | ||
| ['record-verify-passed'], | ||
| ['transition', 'REVIEWING'], | ||
| ['record-review-passed'], | ||
| ['transition', 'SUBMITTING_PR'], | ||
| ['record-pr', '1'], | ||
| ['transition', 'AWAITING_CI'], | ||
| ['record-ci-passed'], | ||
| ['transition', 'CHECKING_FEEDBACK'], | ||
| ['record-feedback-clean'], | ||
| ['transition', 'REFLECTING'], | ||
| ], | ||
| } | ||
|
|
||
| runWorkflow(['init'], deps) | ||
| const steps = stateSteps[targetState] | ||
| if (!steps) return | ||
| for (const step of steps) { | ||
| runWorkflow(step, deps) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make progressToState() fail fast.
Unknown targets currently no-op, and failed setup commands are ignored. That leaves tests in the wrong state and shifts the real failure to some later assertion. Hoist the step table to a const, type targetState as keyof typeof stateSteps, and throw as soon as runWorkflow(['init'], deps) or any preparation step returns a non-zero exit code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts`
around lines 69 - 133, The function progressToState silently no-ops on unknown
targets and ignores non-zero exit codes; hoist the stateSteps table into a const
(e.g., const stateSteps = {...}) outside progressToState, change the function
signature to progressToState(deps: AdapterDeps, targetState: keyof typeof
stateSteps) to type-check targets, and after calling runWorkflow(['init'], deps)
and after each runWorkflow(step, deps) check the returned exit code and throw an
Error (or rethrow) immediately if it is non-zero; also throw immediately if
targetState is not present (defensive check) so failures fail fast.
Owner
Author
There was a problem hiding this comment.
Accepted and fixed. progressToState() now asserts each step's exitCode via expect().toStrictEqual(0), failing immediately on setup errors instead of silently leaving tests in the wrong state.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 1 seconds before sending another message.
Comment on lines
+65
to
+69
| it('records an issue number', () => { | ||
| const deps = setup() | ||
| runWorkflow(['init'], deps) | ||
| const result = runWorkflow(['record-issue', '42'], deps) | ||
| expect(result.exitCode).toStrictEqual(0) |
There was a problem hiding this comment.
Assert the recorded effect, not only exitCode.
These cases are titled records ..., but they only prove the command returned 0. A handler that no-ops and still reports success would pass. Add one observable assertion per command—for example, the guarded transition it should unlock (record-verify-passed → transition REVIEWING, record-pr → transition AWAITING_CI, record-reflection → transition COMPLETE) or a direct state / event-store assertion.
Based on learnings: "Verify that test assertions actually exercise the behavior claimed by the test name. Read the test title, then check that assertions directly verify the claim, not proxy assertions that might pass with wrong implementations."
Also applies to: 89-93, 105-109, 114-118, 130-134, 139-143, 148-152, 171-175, 180-184, 196-200, 205-209, 228-232, 237-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.ts` around lines 65 -
69, The tests only assert exitCode; update each test (using setup() and
runWorkflow(..., deps)) to assert the actual recorded effect: after
runWorkflow(['record-issue','42'], deps) assert the event store or state (e.g.,
deps.eventStore or deps.stateMachine) contains an IssueRecorded event with
number 42; for runWorkflow(['record-verify-passed'], deps) assert the workflow
transitioned to REVIEWING; for runWorkflow(['record-pr', ...], deps) assert
transition to AWAITING_CI; for runWorkflow(['record-reflection', ...], deps)
assert transition to COMPLETE — locate and use the helpers in the spec (setup,
runWorkflow) and the injected deps (event store / state machine) to make one
explicit observable assertion per test instead of only checking exitCode.
Owner
Author
There was a problem hiding this comment.
Accepted and fixed. Added output content assertions (e.g., expect(result.output).toContain('Record issue')) alongside exitCode checks.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 4 seconds before sending another message.
Comment on lines
+3
to
+9
| The task is done. PR is merged or ready for human review. | ||
|
|
||
| ## TODO | ||
|
|
||
| - [ ] Summarise what was delivered: feature built, PR link, key technical decisions | ||
| - [ ] List any follow-up work if applicable | ||
| - [ ] Notify the user the PR is ready for their review |
There was a problem hiding this comment.
Make the final notification conditional on merge status.
Line 3 says COMPLETE also covers already-merged PRs, but Line 9 always tells the agent to say the PR is ready for review. That message is wrong once the PR has already been merged. Use a generic completion message here, or split the step by PR status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/states/complete.md` around lines 3 - 9, Update the
final notification in the complete.md TODO so it’s conditional on the PR merge
status: change the "Notify the user the PR is ready for their review" checklist
item (the final notification step) to either emit a generic completion message
or split it into two outcomes (one message for "PR merged" and a different "PR
ready for review" message) and ensure the wording depends on PR status rather
than always saying "ready for review."
Owner
Author
There was a problem hiding this comment.
Accepted and fixed. Notification is now conditional: "If the PR is not yet merged, notify the user the PR is ready for their review."
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 46 seconds before sending another message.
Comment on lines
+8
to
+15
| - [ ] Create the PR: `gh pr create --title "<title>" --body "<description>"` (or update existing) | ||
| - [ ] Record the PR: `/dev-workflow-v2:workflow record-pr <PR_NUMBER> [PR_URL]` | ||
| - [ ] Transition to AWAITING_CI: `/dev-workflow-v2:workflow transition AWAITING_CI` | ||
|
|
||
| ## Constraints | ||
|
|
||
| - `git push` and `gh pr` are ALLOWED in this state (exempted from the global block) | ||
| - Cannot transition to AWAITING_CI unless prNumber is recorded |
There was a problem hiding this comment.
Make the instructions command-accurate and user-facing.
gh pr create does not cover the “update existing” path, and the blanket “ALLOWED” wording implies every git push/gh pr variant is exempt. Also, prNumber leaks an internal field name into operator-facing guidance.
✏️ Proposed doc fix
-- [ ] Create the PR: `gh pr create --title "<title>" --body "<description>"` (or update existing)
+- [ ] Create the PR: `gh pr create --title "<title>" --body "<description>"`
+- [ ] Update the existing PR if needed: `gh pr edit <PR_NUMBER> --title "<title>" --body "<description>"`
…
-- `git push` and `gh pr` are ALLOWED in this state (exempted from the global block)
-- Cannot transition to AWAITING_CI unless prNumber is recorded
+- Non-blocked `git push` and `gh pr` commands are allowed in this state
+- Cannot transition to AWAITING_CI unless the PR number is recordedAs per coding guidelines, **/*.{ts,tsx,js,jsx,md,yml,yaml}: Use domain terminology from docs/architecture/domain-terminology/contextive/definitions.glossary.yml and add new terms when clarifying domain concepts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/states/submitting-pr.md` around lines 8 - 15, Update
the user-facing checklist and constraints to be command-accurate: change the
"Create the PR" line to show both create and update flows (e.g., keep the
existing example `gh pr create --title "<title>" --body "<description>"` and add
an explicit update example or note such as using `gh pr edit <PR_NUMBER> --title
... --body ...`), clarify the allowed commands entry so it only exempts the
specific commands used in this workflow (reference the existing lines mentioning
`git push` and `gh pr`) rather than implying all variants are allowed, and
replace the internal field name `prNumber` with user-facing wording like "PR
number" in the constraint about transitioning to AWAITING_CI (reference the
`Record the PR: /dev-workflow-v2:workflow record-pr <PR_NUMBER> [PR_URL]` and
`Transition to AWAITING_CI: /dev-workflow-v2:workflow transition AWAITING_CI`
lines).
Owner
Author
There was a problem hiding this comment.
Accepted and fixed. Split create/update into separate steps (gh pr create and gh pr edit), clarified allowed command wording, replaced internal prNumber with user-facing "PR number".
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 46 seconds before sending another message.
Comment on lines
+7
to
+11
| - [ ] Run `pnpm verify` to execute lint, build, typecheck, test, and knip across the monorepo | ||
| - [ ] If verify passes: `/dev-workflow-v2:workflow record-verify-passed` | ||
| - [ ] If verify fails: fix the errors, commit, then `/dev-workflow-v2:workflow record-verify-failed` | ||
| - [ ] If passed — transition to REVIEWING: `/dev-workflow-v2:workflow transition REVIEWING` | ||
| - [ ] If failed — transition back to IMPLEMENTING: `/dev-workflow-v2:workflow transition IMPLEMENTING` |
There was a problem hiding this comment.
Record the failed verification before making new code changes.
Line 9 tells the agent to fix and commit before record-verify-failed. That makes the failure event describe code that has already changed and turns VERIFYING into another implementation pass. Record the failed gate first, transition back to IMPLEMENTING, then fix and re-run verification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/states/verifying.md` around lines 7 - 11, Update the
VERIFYING workflow steps so a failed verification is recorded before any code
changes: when `pnpm verify` fails first call `/dev-workflow-v2:workflow
record-verify-failed`, then call `/dev-workflow-v2:workflow transition
IMPLEMENTING`, and only after those two actions proceed to fix the errors,
commit, and re-run `pnpm verify`; adjust the lines referencing
`/dev-workflow-v2:workflow record-verify-failed` and the transition order
accordingly.
Owner
Author
There was a problem hiding this comment.
Obsolete. File deleted — VERIFYING state was removed.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 42 seconds before sending another message.
Comment on lines
+3
to
+21
| export default defineConfig({ | ||
| test: { | ||
| globals: true, | ||
| coverage: { | ||
| reportsDirectory: './test-output/vitest/coverage', | ||
| provider: 'v8', | ||
| include: ['src/**/*.ts'], | ||
| exclude: [ | ||
| 'src/**/*.spec.ts', | ||
| 'src/**/*-test-fixtures.ts', | ||
| ], | ||
| thresholds: { | ||
| lines: 100, | ||
| statements: 100, | ||
| functions: 100, | ||
| branches: 100, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read its contents
cat -n tools/dev-workflow-v2/vitest.config.mts 2>/dev/null || echo "File not found"Repository: NTCoding/living-architecture
Length of output: 684
🏁 Script executed:
# Also check the reference vitest config in tools/dev-workflow
cat -n tools/dev-workflow/vitest.config.mts 2>/dev/null || echo "File not found"Repository: NTCoding/living-architecture
Length of output: 1186
🏁 Script executed:
# List files in tools directory to understand structure
ls -la tools/ | head -20Repository: NTCoding/living-architecture
Length of output: 309
Set root: __dirname so this config resolves against the workspace.
Without root, the coverage include path and reportsDirectory are resolved relative to the current working directory instead of the tools/dev-workflow-v2 package. In monorepo runs, this causes coverage scans to target the wrong src/** tree and emit reports to the wrong location, breaking coverage enforcement for this workspace.
Suggested fix
export default defineConfig({
+ root: __dirname,
test: {
globals: true,
coverage: {📝 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
| export default defineConfig({ | |
| test: { | |
| globals: true, | |
| coverage: { | |
| reportsDirectory: './test-output/vitest/coverage', | |
| provider: 'v8', | |
| include: ['src/**/*.ts'], | |
| exclude: [ | |
| 'src/**/*.spec.ts', | |
| 'src/**/*-test-fixtures.ts', | |
| ], | |
| thresholds: { | |
| lines: 100, | |
| statements: 100, | |
| functions: 100, | |
| branches: 100, | |
| }, | |
| }, | |
| }, | |
| export default defineConfig({ | |
| root: __dirname, | |
| test: { | |
| globals: true, | |
| coverage: { | |
| reportsDirectory: './test-output/vitest/coverage', | |
| provider: 'v8', | |
| include: ['src/**/*.ts'], | |
| exclude: [ | |
| 'src/**/*.spec.ts', | |
| 'src/**/*-test-fixtures.ts', | |
| ], | |
| thresholds: { | |
| lines: 100, | |
| statements: 100, | |
| functions: 100, | |
| branches: 100, | |
| }, | |
| }, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/dev-workflow-v2/vitest.config.mts` around lines 3 - 21, The Vitest
config lacks a root setting so coverage paths resolve from the process CWD; add
root: __dirname at the top-level of the exported config (the object passed to
defineConfig) so test.coverage.include and test.coverage.reportsDirectory are
resolved relative to this package; update the object returned by defineConfig
(the export default) to include root: __dirname alongside the existing test and
coverage properties.
Owner
Author
There was a problem hiding this comment.
Accepted and fixed. Added root: import.meta.dirname so vitest resolves paths against the workspace directory.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 43 seconds before sending another message.
Remove duplicate SQLite event store — now imported from @ntcoding/agentic-workflow-builder/event-store. Removes direct better-sqlite3 dependency from this project. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts (2)
69-134:⚠️ Potential issue | 🟠 MajorMake
progressToState()fail fast on bad targets and failed commands.This helper ignores the
exitCodefromrunWorkflow(['init'], deps)and every subsequent step, and it silently no-ops whentargetStateis unknown. A typo or invalid transition will leave the fixture in the wrong state and push the real failure into later assertions.Suggested direction
-export function progressToState(deps: AdapterDeps, targetState: string): void { - const stateSteps: Readonly<Record<string, readonly (readonly string[])[]>> = { +const STATE_STEPS = { VERIFYING: [['transition', 'VERIFYING']], REVIEWING: [['transition', 'VERIFYING'], ['record-verify-passed'], ['transition', 'REVIEWING']], SUBMITTING_PR: [ ['transition', 'VERIFYING'], ['record-verify-passed'], @@ REFLECTING: [ ['transition', 'VERIFYING'], ['record-verify-passed'], ['transition', 'REVIEWING'], ['record-review-passed'], @@ ['record-feedback-clean'], ['transition', 'REFLECTING'], ], - } +} as const + +export function progressToState( + deps: AdapterDeps, + targetState: keyof typeof STATE_STEPS, +): void { + const initResult = runWorkflow(['init'], deps) + if (initResult.exitCode !== 0) { + throw new Error(`Failed to initialize workflow fixture: ${initResult.output}`) + } - runWorkflow(['init'], deps) - const steps = stateSteps[targetState] - if (!steps) return + const steps = STATE_STEPS[targetState] for (const step of steps) { - runWorkflow(step, deps) + const result = runWorkflow(step, deps) + if (result.exitCode !== 0) { + throw new Error(`Failed progressing fixture with "${step.join(' ')}": ${result.output}`) + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts` around lines 69 - 134, progressToState ignores runWorkflow exit codes and silently returns on unknown target; make it fail fast by validating targetState exists in stateSteps and by checking the return/exitCode of runWorkflow calls (including the initial runWorkflow(['init'], deps)) and throwing or asserting on any non-zero/failed result so typos or bad transitions immediately surface; update progressToState to lookup stateSteps[targetState] and throw for missing target, then iterate steps calling runWorkflow(step, deps) and throw if runWorkflow indicates failure.
22-24:⚠️ Potential issue | 🟠 MajorExpose a teardown path for the SQLite-backed store before deleting files.
buildTestDeps()creates the event store, but callers only get{ deps, dbPath }.cleanupDb()then unlinks the SQLite files without any way to close the store first. That is a file-handle leak on Unix and a likely cleanup failure on Windows. Return the store or a dedicateddispose()so tests can close it beforecleanupDb()runs.Also applies to: 62-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts` around lines 22 - 24, buildTestDeps currently creates the SQLite-backed store via createStore(dbPath) but only returns { deps, dbPath }, causing cleanupDb to unlink files while the store may still hold open file handles; modify buildTestDeps to also return the created store (or a dispose/close function) so callers can close it before cleanupDb runs, and update cleanupDb to expect and invoke store.close()/store.dispose() (or call the provided dispose function) prior to unlinking the db files; reference createStore, store, buildTestDeps, and cleanupDb when making the change.tools/dev-workflow-v2/src/shell/composition-root.ts (1)
33-36:⚠️ Potential issue | 🟠 MajorWire
checkPrChecksto a real implementation or fail fast.The real dependency graph currently reports every PR as passing checks.
WorkflowDeps.checkPrChecksfeedsprChecksPassinto the transition context, so any CI/PR gate built on that signal will be silently bypassed in production. If the adapter is not ready yet, throw here instead of returningtrue.Suggested fail-fast fallback
const workflowDeps: WorkflowDeps = { getGitInfo, - checkPrChecks: () => true, + checkPrChecks: () => { + throw new Error('buildRealDeps(): checkPrChecks is not wired to a real adapter') + }, now: () => new Date().toISOString(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/shell/composition-root.ts` around lines 33 - 36, The current WorkflowDeps object sets checkPrChecks to a stub that always returns true, which bypasses PR/CI gates; replace this stub in WorkflowDeps with the real adapter implementation that computes prChecksPass (e.g., call the existing PR checks function provided elsewhere in the codebase) or, if that adapter is not yet available, change checkPrChecks to immediately throw a clear error (e.g., throw new Error('checkPrChecks adapter not implemented')) so the system fails fast; update references to WorkflowDeps.checkPrChecks and ensure prChecksPass is sourced from that real function rather than the always-true stub.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@knip.json`:
- Around line 35-45: The knip ignore rule for "@types/better-sqlite3" is masking
a stale type dependency; open the package.json for the tools/dev-workflow-v2
package and remove the "@types/better-sqlite3" entry from its
dependencies/devDependencies, then delete the corresponding "ignoreDependencies"
entry in knip.json under the "tools/dev-workflow-v2" block so the
unused-dependency will be detected and removed by CI/linters; reference the
"tools/dev-workflow-v2" key and the "ignoreDependencies" array when making the
change.
In `@pnpm-workspace.yaml`:
- Around line 8-9: The workspace config currently contradicts itself: the
dependency better-sqlite3 appears in onlyBuiltDependencies while package.json
lists it in ignoredBuiltDependencies; pick one behavior and consolidate
duplicates. Either remove better-sqlite3 from ignoredBuiltDependencies in
package.json so it will be built, or delete the onlyBuiltDependencies block (or
remove better-sqlite3 from it) so it remains ignored; also remove the duplicate
onlyBuiltDependencies entry from package.json and keep a single canonical
onlyBuiltDependencies definition (the one in pnpm-workspace.yaml) to avoid
union/duplicate script execution.
In `@tools/dev-workflow-v2/package.json`:
- Line 15: Remove the unused `@types/better-sqlite3` dependency from package.json
by deleting the "@types/better-sqlite3" entry and then update knip.json to
remove the corresponding ignoreDependencies entry for this project so the
dependency list and knip exceptions stay in sync; search for the
"@types/better-sqlite3" string in package.json and for this project's entry
under ignoreDependencies in knip.json to locate the exact spots to edit.
---
Duplicate comments:
In `@tools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.ts`:
- Around line 69-134: progressToState ignores runWorkflow exit codes and
silently returns on unknown target; make it fail fast by validating targetState
exists in stateSteps and by checking the return/exitCode of runWorkflow calls
(including the initial runWorkflow(['init'], deps)) and throwing or asserting on
any non-zero/failed result so typos or bad transitions immediately surface;
update progressToState to lookup stateSteps[targetState] and throw for missing
target, then iterate steps calling runWorkflow(step, deps) and throw if
runWorkflow indicates failure.
- Around line 22-24: buildTestDeps currently creates the SQLite-backed store via
createStore(dbPath) but only returns { deps, dbPath }, causing cleanupDb to
unlink files while the store may still hold open file handles; modify
buildTestDeps to also return the created store (or a dispose/close function) so
callers can close it before cleanupDb runs, and update cleanupDb to expect and
invoke store.close()/store.dispose() (or call the provided dispose function)
prior to unlinking the db files; reference createStore, store, buildTestDeps,
and cleanupDb when making the change.
In `@tools/dev-workflow-v2/src/shell/composition-root.ts`:
- Around line 33-36: The current WorkflowDeps object sets checkPrChecks to a
stub that always returns true, which bypasses PR/CI gates; replace this stub in
WorkflowDeps with the real adapter implementation that computes prChecksPass
(e.g., call the existing PR checks function provided elsewhere in the codebase)
or, if that adapter is not yet available, change checkPrChecks to immediately
throw a clear error (e.g., throw new Error('checkPrChecks adapter not
implemented')) so the system fails fast; update references to
WorkflowDeps.checkPrChecks and ensure prChecksPass is sourced from that real
function rather than the always-true stub.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0d9a96c-1e2a-4a2e-96bf-10c835b75b70
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
knip.jsonpnpm-workspace.yamltools/dev-workflow-v2/package.jsontools/dev-workflow-v2/src/entrypoint/fixtures/workflow-cli-test-fixtures.tstools/dev-workflow-v2/src/shell/composition-root.ts
Comment on lines
+35
to
+45
| "tools/dev-workflow-v2": { | ||
| "entry": [ | ||
| "src/entrypoint/workflow-cli.ts" | ||
| ], | ||
| "project": [ | ||
| "src/**/*.ts" | ||
| ], | ||
| "ignoreDependencies": [ | ||
| "@types/better-sqlite3" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if `@types/better-sqlite3` is in package.json and if it's actually imported
echo "=== Checking package.json for `@types/better-sqlite3` ==="
if [ -f "tools/dev-workflow-v2/package.json" ]; then
cat tools/dev-workflow-v2/package.json | jq '.devDependencies["@types/better-sqlite3"] // .dependencies["@types/better-sqlite3"] // "NOT FOUND"'
else
echo "tools/dev-workflow-v2/package.json not found"
# Try root package.json
echo "Checking root package.json..."
cat package.json | jq '.devDependencies["@types/better-sqlite3"] // .dependencies["@types/better-sqlite3"] // "NOT FOUND"'
fi
echo ""
echo "=== Searching for imports/references to better-sqlite3 in tools/dev-workflow-v2 ==="
find tools/dev-workflow-v2 -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" \) -exec grep -l "better-sqlite3" {} \; 2>/dev/null || echo "No direct imports found"
echo ""
echo "=== Checking knip.json structure ==="
cat knip.json | jq '.workspaces["tools/dev-workflow-v2"]'Repository: NTCoding/living-architecture
Length of output: 414
🏁 Script executed:
#!/bin/bash
# Double-check for any transitive dependencies or other patterns that might reference better-sqlite3
echo "=== Checking for any 'sqlite' references in tools/dev-workflow-v2 ==="
rg -i "sqlite" tools/dev-workflow-v2/src/ || echo "No sqlite references found"
echo ""
echo "=== Verifying this is the root package.json dependency ==="
cat package.json | jq '.devDependencies["@types/better-sqlite3"], .dependencies["@types/better-sqlite3"]'Repository: NTCoding/living-architecture
Length of output: 238
Remove @types/better-sqlite3 from tools/dev-workflow-v2/package.json instead of ignoring it in knip.
The dependency is not imported or used anywhere in the workspace. Since the better-sqlite3 dependency was removed by consuming the event store from the library, the type package should also be removed from package.json rather than masked with a knip ignore rule. Ignoring unused dependencies prevents automated tooling from catching stale dependencies that should be cleaned up.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knip.json` around lines 35 - 45, The knip ignore rule for
"@types/better-sqlite3" is masking a stale type dependency; open the
package.json for the tools/dev-workflow-v2 package and remove the
"@types/better-sqlite3" entry from its dependencies/devDependencies, then delete
the corresponding "ignoreDependencies" entry in knip.json under the
"tools/dev-workflow-v2" block so the unused-dependency will be detected and
removed by CI/linters; reference the "tools/dev-workflow-v2" key and the
"ignoreDependencies" array when making the change.
Owner
Author
There was a problem hiding this comment.
Not actioned. @types/better-sqlite3 is still required as a devDependency because the platform package's TypeScript source is resolved directly by tsc and imports better-sqlite3. The knip ignore rule remains necessary.
There was a problem hiding this comment.
Rate Limit Exceeded
@NTCoding have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 40 seconds before sending another message.
pnpm v10 onlyBuiltDependencies does not reliably trigger better-sqlite3's native compilation during install. Explicit rebuild ensures bindings exist for dev-workflow-v2 tests. Co-Authored-By: Claude Opus 4.6 <[email protected]>
pnpm v10 onlyBuiltDependencies does not reliably trigger better-sqlite3's native compilation during install. Explicit rebuild ensures bindings exist for dev-workflow-v2 tests. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 45-47: Replace the broad pnpm rebuild step with a scoped rebuild
for better-sqlite3 by changing the unscoped invocation (pnpm rebuild) to pnpm
rebuild better-sqlite3 in the workflow job that needs native bindings; also
review the other jobs named browser-tests, knip, lint-markdown, and publish and
add the same scoped pnpm rebuild better-sqlite3 step immediately after their
pnpm install --frozen-lockfile if those jobs execute code requiring
better-sqlite3 at runtime, otherwise omit the rebuild to avoid unnecessary work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95574ea5-4647-4da2-aa7d-1572b31a5930
📒 Files selected for processing (1)
.github/workflows/ci.yml
pnpm v10 onlyBuiltDependencies does not reliably trigger better-sqlite3's native compilation during install. Explicit rebuild ensures bindings exist for dev-workflow-v2 tests. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Strip manufactured transition hints and state-dependent content from output-messages. Replace with a pure op-name formatter (hyphen-to-space + capitalize). Move from infra/ to domain/ since it's domain logic. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…a root .claude/commands Add root .claude/commands/implement-feature.md that Claude Code discovers natively. Move agents to .claude/agents/ under the plugin dir and register via additionalDirectories. Remove v1 dev-workflow hooks from settings. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Create marketplace at tools/.claude-plugin/ referencing the plugin at tools/dev-workflow-v2/. Add plugin.json, workflow command, and move agents/commands back to plugin root for proper discovery. Remove root .claude/commands/implement-feature.md in favor of plugin-namespaced /dev-workflow-v2:implement-feature. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ts to .claude-plugin/ $CLAUDE_PLUGIN_ROOT resolves to the .claude-plugin/ directory, not the plugin root. Use $CLAUDE_PLUGIN_ROOT/.. to reach the actual plugin root where src/ lives. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (7)
tools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.ts (1)
8-23:⚠️ Potential issue | 🟡 MinorAssert the event-specific payload fields in the happy-path cases.
Most success-path cases still only verify
result.type, so a schema regression that drops or renames fields likerepository,issueNumber,branch,passed,prUrl,command,filePath, orfrom/tocan stay green.Based on learnings: "Verify that test assertions actually exercise the behavior claimed by the test name. Read the test title, then check that assertions directly verify the claim, not proxy assertions that might pass with wrong implementations."
Also applies to: 27-34, 47-54, 67-84, 97-114, 127-144, 157-174, 187-203, 217-223, 231-237, 251-256, 265-285, 301-321, 337-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.ts` around lines 8 - 23, The tests only assert result.type and risk letting schema regressions slip; update each happy-path test using WORKFLOW_EVENT_SCHEMA.parse (e.g., the 'accepts valid payload' and 'accepts optional repository' cases and the other listed cases) to also assert the event-specific fields returned by the parse call (for example assert result.repository === 'owner/repo', result.issueNumber, result.branch, result.passed, result.prUrl, result.command, result.filePath, and result.from/result.to as applicable) so the assertions verify the exact payload shape rather than only result.type.tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts (3)
253-255:⚠️ Potential issue | 🟠 MajorPreserve the
PreconditionResultcontract for invalid target states.
parseStateName(target)throws beforetransitionTo()can return a failed result. Direct callers then have to handle a parse exception instead of the samefail(...)path used for every other transition failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts` around lines 253 - 255, transitionTo currently calls parseStateName(target) which can throw and bypass returning a PreconditionResult; update transitionTo to validate/parse the target inside a try/catch and when parseStateName(target) throws return a failed PreconditionResult (use PreconditionResult.fail(...)) with an appropriate message/error instead of letting the exception propagate, preserving the same failure contract as other transition failures; reference the transitionTo method and parseStateName and ensure callers always receive a PreconditionResult rather than an exception.
42-43:⚠️ Potential issue | 🟠 MajorDo not expose live workflow state or event buffers.
createFresh()reuses the sharedEMPTY_STATE, and both getters return mutable references. Callers can mutate workflow state or pending events without emitting an event, which breaks the event-sourced invariants.🧱 Suggested hardening
static createFresh(deps: WorkflowDeps): Workflow { - return new Workflow(EMPTY_STATE, deps) + return new Workflow({ ...EMPTY_STATE }, deps) } @@ getPendingEvents(): readonly WorkflowEvent[] { - return this.pendingEvents + return structuredClone(this.pendingEvents) } @@ getState(): WorkflowState { - return this.state + return { ...this.state } }Also applies to: 54-55, 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts` around lines 42 - 43, createFresh currently returns a Workflow constructed with the shared EMPTY_STATE which exposes live mutable state and event buffers; change createFresh to pass an immutable or defensive copy of EMPTY_STATE (deep clone or Object.freeze) so callers cannot mutate the shared object, and update the Workflow getters that currently return live references (e.g., the state and pending-events getters) to return copies or read-only views instead; ensure any other constructors or static factories (including the ones at the other noted locations) follow the same pattern so no consumer can mutate internal state or pending event buffers directly.
79-87:⚠️ Potential issue | 🟠 MajorValidate issue and PR numbers before appending them to the event stream.
These methods currently accept
0, negatives, fractions, andNaN. That lets impossible identifiers into persisted workflow history and can leave the workflow in an invalid or misleading state.🛡️ Suggested validation
recordIssue(issueNumber: number): PreconditionResult { const gate = checkOperationGate('record-issue', this.state) if (!gate.pass) return gate + if (!Number.isInteger(issueNumber) || issueNumber <= 0) { + return fail('Issue number must be a positive integer.') + } this.append({ type: 'issue-recorded', at: this.deps.now(), issueNumber, @@ recordPr(prNumber: number, prUrl?: string): PreconditionResult { const gate = checkOperationGate('record-pr', this.state) if (!gate.pass) return gate + if (!Number.isInteger(prNumber) || prNumber <= 0) { + return fail('PR number must be a positive integer.') + } this.append({ type: 'pr-recorded', at: this.deps.now(), prNumber,Also applies to: 157-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts` around lines 79 - 87, The recordIssue method currently appends any numeric input (0, negatives, fractions, NaN) to the event stream; validate the input before append by ensuring issueNumber is a finite positive integer (e.g., Number.isInteger(issueNumber) && isFinite(issueNumber) && issueNumber > 0) and return a failing PreconditionResult (same shape as checkOperationGate) when validation fails instead of appending; apply the identical validation to the corresponding PR-recording method (the sibling method that records PR numbers) so neither method persists invalid identifiers.tools/dev-workflow-v2/src/entrypoint/workflow-cli.ts (1)
71-75:⚠️ Potential issue | 🟠 MajorDon't let the catch-path logger throw.
appendFileSync(getErrorLogPath(), message)can fail when the log directory is missing, which masks the original exception you were trying to report.🛠️ Suggested hardening
-import { appendFileSync } from 'node:fs' +import { appendFileSync, mkdirSync } from 'node:fs' +import { dirname } from 'node:path' @@ } catch (error) { const message = `[${new Date().toISOString()}] HOOK ERROR: ${String(error)}\n` process.stderr.write(message) - appendFileSync(getErrorLogPath(), message) + try { + const errorLogPath = getErrorLogPath() + mkdirSync(dirname(errorLogPath), { recursive: true }) + appendFileSync(errorLogPath, message) + } catch { + // Do not mask the original failure while reporting it. + } process.exit(EXIT_ERROR) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.ts` around lines 71 - 75, The catch-path logging can itself throw (e.g., appendFileSync(getErrorLogPath(), message) fails if the log directory is missing); update the catch block around the HOOK ERROR handling to guard file operations: before calling appendFileSync(getErrorLogPath(), message) ensure the directory for getErrorLogPath() exists (mkdirSync(..., { recursive: true })) or wrap appendFileSync in its own try/catch and on failure fallback to writing the message to stderr only; keep the original process.exit(EXIT_ERROR) behavior and reference the same symbols (appendFileSync, getErrorLogPath, process.stderr.write, EXIT_ERROR) when making the change.tools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.ts (2)
39-43:⚠️ Potential issue | 🟠 MajorAssert the recorded effect, not just
exitCode.A handler that returns success without recording anything would still satisfy these cases. Please assert one observable outcome per command—persisted event, updated state, or unlocked next transition—so the test title is actually exercised.
Based on learnings: "Verify that test assertions actually exercise the behavior claimed by the test name. Read the test title, then check that assertions directly verify the claim, not proxy assertions that might pass with wrong implementations."
Also applies to: 65-69, 89-93, 105-109, 114-118, 130-134, 139-143, 148-152, 157-161, 180-184, 189-193, 205-209, 214-218, 237-241, 246-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.ts` around lines 39 - 43, The test currently only checks exitCode for runWorkflow(['init'], deps) so update the 'starts a session' test to assert the actual recorded effect (e.g., that the store or event log was written and session state updated) rather than just exitCode: locate the test using setup() and runWorkflow and add an assertion against the mock persistence or event recorder provided in setup (for example inspect deps.store.events or deps.sessionStore.get/state) to confirm a "session started" event or updated session state exists; apply the same pattern to the other listed tests (lines referenced) so each test verifies the observable outcome (persisted event, updated state, or unlocked transition) produced by the specific command handler rather than only exitCode.
164-176:⚠️ Potential issue | 🟠 MajorValidate arguments after entering the correct state first.
record-pr,record-feedback-exists, andrecord-reflectionare state-gated commands. With onlyinit, these cases can fail before argument parsing runs, so they don't prove the validation path. Drive the workflow toSUBMITTING_PR,CHECKING_FEEDBACK, andREFLECTINGfirst, then assert the specific validation message.Based on learnings: "Verify that test assertions actually exercise the behavior claimed by the test name. Read the test title, then check that assertions directly verify the claim, not proxy assertions that might pass with wrong implementations."
Also applies to: 221-233, 253-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.ts` around lines 164 - 176, The tests currently call runWorkflow(['init'], deps) then directly invoke state-gated commands (record-pr, record-feedback-exists, record-reflection) so argument validation is never reached; update each test to first drive the workflow into the appropriate state with runWorkflow (e.g., call whatever command(s) via runWorkflow that transition the workflow to SUBMITTING_PR before testing record-pr, to CHECKING_FEEDBACK before testing record-feedback-exists, and to REFLECTING before testing record-reflection), then call runWorkflow(['record-pr', 'abc'], deps) (and the analogous bad-arg invocations) and assert both result.exitCode === 1 and that the specific validation message is returned; locate tests using setup and runWorkflow in workflow-cli.spec.ts and update the blocks around the record-pr / record-feedback-exists / record-reflection tests (also apply same fix to the other failing ranges mentioned).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/dev-workflow-v2/agents/architecture-review.md`:
- Around line 11-12: There is a conflicting output contract between the
instruction "You will return structured JSON output with a single field:
`verdict`" and the later requirement to also return full Findings/Audit
Trail/Audit Summary; choose one canonical response shape and update the prompt
text accordingly: either (A) require only the single-field JSON `{ "verdict":
"PASS" | "FAIL" }` and move the Findings/Audit Trail/Audit Summary into a
separate follow-up step or optional metadata field, or (B) require a structured
JSON object that contains `verdict` plus nested fields `findings`, `auditTrail`,
and `auditSummary`; update all occurrences (the single-field line and the blocks
at lines referenced in the comment) so they match the chosen shape and adjust
examples and callers to parse the new contract consistently.
In `@tools/dev-workflow-v2/commands/implement-feature.md`:
- Around line 31-38: The workflow sequence is missing the ADDRESSING_FEEDBACK
state; update the state list so it includes ADDRESSING_FEEDBACK between
CHECKING_FEEDBACK and REFLECTING (i.e., IMPLEMENTING → VERIFYING → REVIEWING →
SUBMITTING_PR → AWAITING_CI → CHECKING_FEEDBACK → ADDRESSING_FEEDBACK →
REFLECTING → COMPLETE) and ensure the enumerated list and any references to
total states (10 states plus BLOCKED) reflect this insertion; modify the
sequence entries (the named states like CHECKING_FEEDBACK, ADDRESSING_FEEDBACK,
REFLECTING, COMPLETE) accordingly so numbering and documentation remain
consistent.
In `@tools/dev-workflow-v2/commands/workflow.md`:
- Around line 7-9: The current one-liner uses export GITHUB_TOKEN=$(gh auth
token) which masks failures of gh auth token; change it to first run gh auth
token and capture its output into GITHUB_TOKEN with a failure check (so a
failing gh call exits immediately), then export GITHUB_TOKEN and run the
workflow CLI; update the lines referencing GITHUB_TOKEN, gh auth token, and the
invocation of workflow-cli.ts (using $CLAUDE_PLUGIN_ROOT and $ARGUMENTS) so the
script exits on authentication failure rather than continuing with an empty
token.
In `@tools/dev-workflow-v2/README.md`:
- Line 42: The diagram line "BLOCKED --> IMPLEMENTING" is incorrect; update the
README so BLOCKED returns to the recorded pre-blocked state instead of
hardcoding IMPLEMENTING — reflect the workflow contract implemented in
tools/dev-workflow-v2/src/workflow-definition/domain/states/blocked.ts (see the
preBlockedState handling). Replace the edge text with something like "BLOCKED
--> preBlockedState (previous state)" and, if present elsewhere in the diagram
or caption, adjust any explanatory text to say blocked workflows resume their
recorded preBlockedState (e.g., VERIFYING, REVIEWING, IMPLEMENTING).
In `@tools/dev-workflow-v2/src/workflow-definition/domain/fold.ts`:
- Around line 4-12: EMPTY_STATE duplicates INITIAL_STATE; remove the second
literal by replacing EMPTY_STATE with a single canonical reference to
INITIAL_STATE from workflow-types.ts (or export INITIAL_STATE from that module
and import it here), then update any usages that reference EMPTY_STATE to use
INITIAL_STATE so applyEvents([]), Workflow.createFresh(), and
WORKFLOW_ADAPTER.initialState() share the same single source of truth.
In
`@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.ts`:
- Around line 36-42: The tests are asserting Zod's exact error message strings
(e.g., in the test that calls WORKFLOW_EVENT_SCHEMA.parse and expects
toThrow('Required') and similar for discriminator errors); change these
assertions to avoid pinning Zod's stock messages by either using expect(() =>
WORKFLOW_EVENT_SCHEMA.parse(...)).toThrow() with no message match or by catching
the thrown ZodError and asserting on its issues array (checking issue.path and
issue.code) instead; update the failing tests that reference
WORKFLOW_EVENT_SCHEMA.parse and any expecting 'Required' or 'Invalid
discriminator value' to use one of these approaches so they test
behavior/stucture rather than Zod's exact text.
In
`@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-reviewing-submitting.spec.ts`:
- Around line 113-120: The test only asserts preBlockedState but not the actual
state transition; update the BLOCKED transition tests (the one using
spec.given(...eventsToReviewing()).when((wf) => wf.transitionTo('BLOCKED')) and
the other similar cases at the referenced ranges) to also assert that the
workflow's currentStateMachineState (e.g., state.currentStateMachineState or
equivalent on the returned state) equals 'BLOCKED' after the transition; keep
the existing preBlockedState assertion and add a direct expect that the current
state is 'BLOCKED' to ensure the transition occurred.
- Around line 67-83: The test currently only checks that a 'review-completed'
event with passed: false was emitted; update it to also assert the failed
reviewer list so the contract of recordReviewFailed is exercised. After calling
recordReviewFailed(['code-review']), assert that the resulting state
(state.failedReviewers) and the emitted event payload include failedReviewers:
['code-review'] (e.g., find the event with type 'review-completed' and check its
failedReviewers field) to ensure recordReviewFailed preserves and emits the
provided reviewer list.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.spec.ts`:
- Around line 85-98: Add a negative test to ensure preBlockedState rejects
invalid names: update the spec to include a new test that constructs a raw
object with preBlockedState set to an invalid string (e.g., 'INVALID_STATE') and
asserts that workflowStateSchema.parse(raw) throws; this verifies the schema
(workflowStateSchema) uses the tighter stateNameSchema.optional() validation for
preBlockedState rather than allowing any string.
---
Duplicate comments:
In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.ts`:
- Around line 39-43: The test currently only checks exitCode for
runWorkflow(['init'], deps) so update the 'starts a session' test to assert the
actual recorded effect (e.g., that the store or event log was written and
session state updated) rather than just exitCode: locate the test using setup()
and runWorkflow and add an assertion against the mock persistence or event
recorder provided in setup (for example inspect deps.store.events or
deps.sessionStore.get/state) to confirm a "session started" event or updated
session state exists; apply the same pattern to the other listed tests (lines
referenced) so each test verifies the observable outcome (persisted event,
updated state, or unlocked transition) produced by the specific command handler
rather than only exitCode.
- Around line 164-176: The tests currently call runWorkflow(['init'], deps) then
directly invoke state-gated commands (record-pr, record-feedback-exists,
record-reflection) so argument validation is never reached; update each test to
first drive the workflow into the appropriate state with runWorkflow (e.g., call
whatever command(s) via runWorkflow that transition the workflow to
SUBMITTING_PR before testing record-pr, to CHECKING_FEEDBACK before testing
record-feedback-exists, and to REFLECTING before testing record-reflection),
then call runWorkflow(['record-pr', 'abc'], deps) (and the analogous bad-arg
invocations) and assert both result.exitCode === 1 and that the specific
validation message is returned; locate tests using setup and runWorkflow in
workflow-cli.spec.ts and update the blocks around the record-pr /
record-feedback-exists / record-reflection tests (also apply same fix to the
other failing ranges mentioned).
In `@tools/dev-workflow-v2/src/entrypoint/workflow-cli.ts`:
- Around line 71-75: The catch-path logging can itself throw (e.g.,
appendFileSync(getErrorLogPath(), message) fails if the log directory is
missing); update the catch block around the HOOK ERROR handling to guard file
operations: before calling appendFileSync(getErrorLogPath(), message) ensure the
directory for getErrorLogPath() exists (mkdirSync(..., { recursive: true })) or
wrap appendFileSync in its own try/catch and on failure fallback to writing the
message to stderr only; keep the original process.exit(EXIT_ERROR) behavior and
reference the same symbols (appendFileSync, getErrorLogPath,
process.stderr.write, EXIT_ERROR) when making the change.
In
`@tools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.ts`:
- Around line 8-23: The tests only assert result.type and risk letting schema
regressions slip; update each happy-path test using WORKFLOW_EVENT_SCHEMA.parse
(e.g., the 'accepts valid payload' and 'accepts optional repository' cases and
the other listed cases) to also assert the event-specific fields returned by the
parse call (for example assert result.repository === 'owner/repo',
result.issueNumber, result.branch, result.passed, result.prUrl, result.command,
result.filePath, and result.from/result.to as applicable) so the assertions
verify the exact payload shape rather than only result.type.
In `@tools/dev-workflow-v2/src/workflow-definition/domain/workflow.ts`:
- Around line 253-255: transitionTo currently calls parseStateName(target) which
can throw and bypass returning a PreconditionResult; update transitionTo to
validate/parse the target inside a try/catch and when parseStateName(target)
throws return a failed PreconditionResult (use PreconditionResult.fail(...))
with an appropriate message/error instead of letting the exception propagate,
preserving the same failure contract as other transition failures; reference the
transitionTo method and parseStateName and ensure callers always receive a
PreconditionResult rather than an exception.
- Around line 42-43: createFresh currently returns a Workflow constructed with
the shared EMPTY_STATE which exposes live mutable state and event buffers;
change createFresh to pass an immutable or defensive copy of EMPTY_STATE (deep
clone or Object.freeze) so callers cannot mutate the shared object, and update
the Workflow getters that currently return live references (e.g., the state and
pending-events getters) to return copies or read-only views instead; ensure any
other constructors or static factories (including the ones at the other noted
locations) follow the same pattern so no consumer can mutate internal state or
pending event buffers directly.
- Around line 79-87: The recordIssue method currently appends any numeric input
(0, negatives, fractions, NaN) to the event stream; validate the input before
append by ensuring issueNumber is a finite positive integer (e.g.,
Number.isInteger(issueNumber) && isFinite(issueNumber) && issueNumber > 0) and
return a failing PreconditionResult (same shape as checkOperationGate) when
validation fails instead of appending; apply the identical validation to the
corresponding PR-recording method (the sibling method that records PR numbers)
so neither method persists invalid identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52ea13d2-bed5-45e9-9601-30fd2987b869
📒 Files selected for processing (28)
.claude/settings.json.github/workflows/ci.ymltools/.claude-plugin/marketplace.jsontools/dev-workflow-v2/.claude-plugin/plugin.jsontools/dev-workflow-v2/CLAUDE.mdtools/dev-workflow-v2/README.mdtools/dev-workflow-v2/agents/architecture-review.mdtools/dev-workflow-v2/agents/bug-scanner.mdtools/dev-workflow-v2/agents/code-review.mdtools/dev-workflow-v2/agents/task-check.mdtools/dev-workflow-v2/commands/implement-feature.mdtools/dev-workflow-v2/commands/workflow.mdtools/dev-workflow-v2/src/entrypoint/workflow-cli.spec.tstools/dev-workflow-v2/src/entrypoint/workflow-cli.tstools/dev-workflow-v2/src/infra/cli/command-handlers.tstools/dev-workflow-v2/src/workflow-definition/domain/fold.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/fold.tstools/dev-workflow-v2/src/workflow-definition/domain/output-messages.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/output-messages.tstools/dev-workflow-v2/src/workflow-definition/domain/states/reviewing.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-events.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-reviewing-submitting.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.spec.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow-types.tstools/dev-workflow-v2/src/workflow-definition/domain/workflow.tstools/dev-workflow-v2/src/workflow-definition/infra/workflow-adapter.tstools/dev-workflow-v2/states/reviewing.md
Move marketplace.json to plugin root (.claude-plugin/), matching the reference pattern where source "./" means the plugin IS the marketplace. Add SessionStart/PreToolUse hooks for env var injection (CLAUDE_SESSION_ID, CLAUDE_PLUGIN_ROOT). Simplify workflow.md command to match reference repo. Bump plugin version to 0.0.4. After this commit, register with: /plugin marketplace add ./tools/dev-workflow-v2 /plugin install dev-workflow-v2@dev-workflow-v2-marketplace Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove redundant VERIFYING state (pnpm verify runs via pre-commit hooks). IMPLEMENTING now transitions directly to REVIEWING with guards for workingTreeClean and githubIssue. Replace single reviewPassed flag with individual review agent verdicts (architecture-review, code-review, bug-scanner) — all three must pass before transitioning to SUBMITTING_PR. Add feedback count tracking: auto-fetch PR feedback on entry to CHECKING_FEEDBACK, enforce addressedCount >= unresolvedCount in ADDRESSING_FEEDBACK transition guard. Tighten allowForbidden to block chained commands (e.g. gh pr checks && gh pr merge). Add Write/Edit tool hook to block writes to protected config files (nx.json, tsconfig.base.json, etc). Bump plugin version to 0.0.6. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove consumer-side checkBashAllowed, DANGEROUS_FLAGS, GLOBAL_FORBIDDEN, and ForbiddenBashCommand type. Bash command enforcement now uses the platform's checkBashCommand with typed BashForbiddenConfig. Per-state exemptions (AWAITING_CI: gh pr checks, CHECKING_FEEDBACK: gh pr view) configured via allowForbidden in state definitions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace scattered workflow entry points (.claude/commands/next-task.md, scripts/start-task.sh, commands/implement-feature.md) with two self-contained plugin commands: choose-next-task and start-implementation. User now starts sessions with `claude -w` so the plugin owns zero worktree logic — branch renaming is the only git setup needed. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace hand-wired CLI infrastructure with platform's declarative
defineRoutes/defineHooks/createWorkflowCli APIs. Eliminates ~2,100
lines of custom command handling, hook routing, and composition root
code in favor of ~130 lines of declarative config.
Key changes:
- Add defineRoutes with all 16 recording transaction routes
- Add defineHooks (empty — uses preToolUseHandler for tool checks)
- Extract shell/cli.ts as runtime entry point (separated from testable entrypoint)
- Rename workflow-adapter → workflow-definition (implements WorkflowDefinition interface)
- Add stateOverrides to transitioned events for state reset on entry
- Widen fold to apply stateOverrides generically
- Add forbidden: { write: true } to all state definitions
- Migrate Workflow class to appendEvent + executeRecording pattern
- Delete: command-handlers, hook-handlers, hook-io, environment,
operation-result, stdin, composition-root, workflow-error (10 files)
- Full test coverage: 221 tests, 100% across all files
Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
This was referenced Apr 13, 2026
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
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
dev-workflow-v2: an event-sourced workflow state machine plugin for Claude Code enforcing a structured task lifecycle (implement, verify, review, submit PR, await CI, check feedback, reflect, complete).Documentation
Chores