ship-gated, ship-loop, dev-product-boundary: orchestrator, loop mode, and .dev/ split#19
Merged
Conversation
The approved /plan artifact for the gated /ship orchestrator. Committed
ahead of /regress so the dirty-tree partition resolves to inside={ship.md}
(= the PLAN's ## Files / declared writes), keeping the /plan artifact out
of `inside` and avoiding a false fix#7 escape (pipeline-integration-probe
CF-1). The /build output (.claude/commands/ship.md) is intentionally left
uncommitted as the feature under test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds .claude/commands/ship.md — a gated orchestrator that runs the build loop (/plan → [GATE 1] → /grill → /build → /regress → /verify → /review → [GATE 2]), branching to the next stage only on each stage's STRUCTURAL floor verdict (validate exit / regression-report.verdict / verify-report .verdict), presenting /grill+/review free-text as quoted DATA, ending only at a human gate or a RED-verdict STOP. No role: (capability count stays 1); adds NO new floor primitive — every guarantee belongs to a sub-stage. NO --yolo; --loop deferred to the ship-loop increment (OQ1 split). Pipeline trail (all floor verdicts GREEN): /build floor GREEN · /regress no-regressions · /verify PASS · /review GREEN (0 blocking). Advisory review findings (A-1/A-2): the orchestration LOGIC is floor-invisible and unmechanized until a live dogfood — the standing residual. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The approved /plan artifact for the ship-loop (--loop mode) increment.
Committed ahead of /regress so the dirty-tree partition resolves to
inside = {ship.md, floor/check-ship.mjs, floor/check-ship.test.mjs}
(= the PLAN's ## Files / declared writes), keeping the /plan artifact out
of `inside` and avoiding a false fix#7 escape (pipeline-integration-probe
CF-1; same discipline as ship-gated). The /build output stays uncommitted
as the feature under test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds --loop to /ship: iterates fix → regress → verify → review until a
floor-grade stop — /verify PASS ∧ /regress clean — or a max-iteration cap
(default 3, --max-iter). The stop is computed by the new tested floor
helper floor/check-ship.mjs, whose inputs are ONLY the two floor verdicts
+ iter/cap (exit 0 STOP_GREEN / 1 STOP_CAP / 2 INCONCLUSIVE / 3 CONTINUE).
So "/review never gates the loop" is STRUCTURAL — the helper has no /review
input — not agent discipline (the fix#3 disease made impossible). 12
hermetic tests in floor/check-ship.test.mjs. The loop guarantees the STOP,
never that a fix works; GATE 1 (plan approval) hit once, never re-entered;
no --yolo.
Pipeline trail (all floor verdicts GREEN): /regress no-regressions ·
/verify PASS · /review GREEN (0 blocking). Includes the post-review fix of
REVIEW finding A-3: the CONTINUE step now re-sets the writes-scope
(set-writes-scope --from-plan) before applying a fix, since each stage's
Step 0 setter overwrites .pharn/writes-scope.json (fix#7 does not persist
across stages). Standing advisory residuals: A-1 (agent compliance with the
stop is advisory), A-2 (loop orchestration unverified until a live --loop
dogfood), A-4 (check-ship hardcodes the stage verdict enums).
Floor capability count unchanged (1): ship.md is a no-role command;
check-ship.{mjs,test.mjs} live in floor/ (path-ignored by validate).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mands to pharn-dev- Structural dev/product split so packaging is "root minus .dev/": floor, build-loop features, and memory-bank relocate; commands gain the pharn-dev- prefix; floor excludes .dev/ wholesale with zero behavior change. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughUpdates the dev/product boundary to use ChangesDev/product boundary restructure and pharn-spec
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.dev/features/ship-loop/regression-report.json (1)
1-22: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStale paths in
insidearray — not updated for dev-product-boundary restructure.The
insidearray references pre-restructure paths that don't match the current repository layout:
.claude/commands/ship.md→ should be.claude/commands/pharn-dev-ship.md(renamed topharn-dev-prefix)floor/check-ship.mjs→ should be.dev/floor/check-ship.mjs(moved under.dev/)floor/check-ship.test.mjs→ should be.dev/floor/check-ship.test.mjs(moved under.dev/)Since this audit trail is being committed after the dev-product-boundary restructure (layers 1–6), the regression report should reflect the actual paths on disk at the time of the run, or be regenerated to match the post-restructure layout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.dev/features/ship-loop/regression-report.json around lines 1 - 22, The regression report’s inside array still points to pre-restructure paths that no longer match the current layout. Update the regression report generation/output so the inside entries use the renamed and relocated symbols/files now referenced by the run, specifically the ship command file and the check-ship implementation/test under the .dev floor area, or regenerate the report from the post-restructure repository state so the recorded paths are accurate.
🧹 Nitpick comments (2)
.claude/commands/pharn-dev-verify.md (1)
2-22: 🧹 Nitpick | 🔵 TrivialSame
role:discriminator question as other command docs.The frontmatter lacks a
role:field. If this command doc is meant to be a capability, addrole:per coding guidelines. If it's purely a command spec, this may be intentional.Based on coding guidelines:
**/*.md: Capability markdown files become capabilities only when their frontmatter includes arole:discriminator such asskill,lens,validator,verifier,griller, orauditor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/commands/pharn-dev-verify.md around lines 2 - 22, Add a frontmatter role discriminator to this command doc if it is intended to be a capability; the current metadata in the /pharn-dev-verify document has no role:, which is required by the markdown capability guidelines. Update the frontmatter near the existing description/kind/trust/model_tier block in pharn-dev-verify.md with the appropriate role value (for example verifier) or leave it unchanged only if this file is deliberately just a command spec.Source: Coding guidelines
.claude/commands/pharn-dev-regress.md (1)
2-10: 🧹 Nitpick | 🔵 TrivialCommand docs may need
role:discriminator if they are capabilities.The frontmatter shows
kind: pharn-ownedbut norole:field. Per coding guidelines,**/*.mdcapability markdown files become capabilities only when their frontmatter includes arole:discriminator such asskill,lens,validator,verifier,griller, orauditor. If this command doc is meant to be a capability, addrole:. If it's purely a command spec (not a capability), this may be intentional.Based on coding guidelines:
**/*.md: Capability markdown files become capabilities only when their frontmatter includes arole:discriminator such asskill,lens,validator,verifier,griller, orauditor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/commands/pharn-dev-regress.md around lines 2 - 10, The command frontmatter for pharn-dev-regress is missing a role discriminator, so decide whether this markdown is a capability or just a command spec and update the metadata accordingly. If it should be a capability, add an appropriate role value to the frontmatter in this document so it is recognized by the markdown capability rules; if not, leave it as a non-capability command spec and keep the current metadata intentional. Use the existing frontmatter keys like kind, trust, and model_tier as the place to make the change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/commands/pharn-dev-ship.md:
- Around line 6-17: Add `.dev/features/<name>/PLAN.md` to the `reads:`
frontmatter for `pharn-dev-ship.md`, since the command body in this workflow
uses the plan file to determine the slug and to drive `set-writes-scope.cjs
--from-plan`. Update the documented read surface so it matches the actual
dependencies of the command, keeping the existing `reads`/`writes` structure
consistent with the other feature commands that rely on `PLAN.md`.
- Around line 90-93: The loop flow in /pharn-dev-ship skips the /pharn-dev-build
floor after a CONTINUE, so later iterations can reach /pharn-dev-regress and
/pharn-dev-verify without re-running the build-stage checks. Update the
state-machine steps in this spec so every loop fix routes back through
/pharn-dev-build before regress/verify/review, preserving the validate/spec-hash
gate defined by /pharn-dev-build and the floor-verdict behavior described in
this document. Use the existing /pharn-dev-build, /pharn-dev-regress,
/pharn-dev-verify, /pharn-dev-review, and STOP_GREEN flow references to keep the
sequence consistent.
In @.claude/hooks/enforce-writes-scope.cjs:
- Around line 26-31: The fallback allowlist in DEFAULT_SAFE_SET still grants
write access to root features/** when writes-scope setup fails, which bypasses
the intended dev/product split. Update the enforce-writes-scope.cjs scope
defaults so the generic fallback is fail-closed for product features and only
keeps .dev/features/** (and other truly safe globs) writable by default; if root
features/** must remain writable, grant it explicitly in the separate product
flow rather than through DEFAULT_SAFE_SET.
In @.dev/features/dev-product-boundary/PLAN.md:
- Line 185: The backstop grep example in the PLAN.md entry uses a placeholder
pattern in the grep -E expression that will not match real feature paths. Update
the command so the regex in the no-stale-reference check uses a real
path-matching pattern instead of the literal features/<name> placeholder, or
clearly mark it as a substitution token; use the no-stale-reference grep example
and the surrounding backstop command text to locate the fix.
In @.dev/floor/check-ship.mjs:
- Around line 99-129: The argv parsing in main() is still too permissive because
extra positionals, unknown flags, and duplicate --iter/--cap occurrences can be
ignored, allowing STOP_* or CONTINUE instead of fail-closed behavior. Tighten
the parser around argv, positional, flag(), posInt(), and emit() so any
unexpected argument shape is treated as invalid input: reject extra non-flag
args, reject unrecognized flags, and reject repeated known flags before verdict
evaluation. Make the malformed-invocation path return the same
INCONCLUSIVE/exit-2 handling used for other bad operands, rather than silently
continuing.
In @.dev/floor/validate.mjs:
- Around line 16-20: The floor validator currently accepts an arbitrary
targetDir, which can let `.dev/` or other subtrees be counted as product
surface; update `validate.mjs` so it always validates from the repository root
and keeps the tooling exclusions intact. Adjust the `TARGET`/path resolution and
any related checks in the validator flow (`relative(TARGET, file)`,
`isExcluded()`, and the main scan logic) so `.claude/commands/` and `.dev/` are
always ignored regardless of the caller’s argument.
---
Outside diff comments:
In @.dev/features/ship-loop/regression-report.json:
- Around line 1-22: The regression report’s inside array still points to
pre-restructure paths that no longer match the current layout. Update the
regression report generation/output so the inside entries use the renamed and
relocated symbols/files now referenced by the run, specifically the ship command
file and the check-ship implementation/test under the .dev floor area, or
regenerate the report from the post-restructure repository state so the recorded
paths are accurate.
---
Nitpick comments:
In @.claude/commands/pharn-dev-regress.md:
- Around line 2-10: The command frontmatter for pharn-dev-regress is missing a
role discriminator, so decide whether this markdown is a capability or just a
command spec and update the metadata accordingly. If it should be a capability,
add an appropriate role value to the frontmatter in this document so it is
recognized by the markdown capability rules; if not, leave it as a
non-capability command spec and keep the current metadata intentional. Use the
existing frontmatter keys like kind, trust, and model_tier as the place to make
the change.
In @.claude/commands/pharn-dev-verify.md:
- Around line 2-22: Add a frontmatter role discriminator to this command doc if
it is intended to be a capability; the current metadata in the /pharn-dev-verify
document has no role:, which is required by the markdown capability guidelines.
Update the frontmatter near the existing description/kind/trust/model_tier block
in pharn-dev-verify.md with the appropriate role value (for example verifier) or
leave it unchanged only if this file is deliberately just a command spec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e23f56f-b790-4d21-9f21-fc908add4cab
📒 Files selected for processing (132)
.claude/commands/pharn-dev-build.md.claude/commands/pharn-dev-eval.md.claude/commands/pharn-dev-grill.md.claude/commands/pharn-dev-memory-promote.md.claude/commands/pharn-dev-plan.md.claude/commands/pharn-dev-regress.md.claude/commands/pharn-dev-review.md.claude/commands/pharn-dev-ship.md.claude/commands/pharn-dev-verify.md.claude/hooks/enforce-writes-scope.cjs.claude/hooks/enforce-writes-scope.test.cjs.claude/hooks/set-writes-scope.test.cjs.dev/features/README.md.dev/features/command-artifact-paths/PLAN.md.dev/features/command-artifact-paths/REVIEW.md.dev/features/dev-product-boundary/GRILL.md.dev/features/dev-product-boundary/PLAN.md.dev/features/dev-product-boundary/REGRESSION.md.dev/features/dev-product-boundary/REVIEW.md.dev/features/dev-product-boundary/VERIFY.md.dev/features/dev-product-boundary/regression-report.json.dev/features/dev-product-boundary/verify-report.json.dev/features/eval-format/PLAN.md.dev/features/eval-format/REVIEW.md.dev/features/frontmatter-parse-parity/PLAN.md.dev/features/frontmatter-parse-parity/REVIEW.md.dev/features/grill-command/PLAN.md.dev/features/grill-command/REVIEW.md.dev/features/memory-promote/PLAN.md.dev/features/memory-promote/REVIEW.md.dev/features/pharn-eval/PLAN.md.dev/features/pharn-eval/REVIEW.md.dev/features/pipeline-integration-probe/GRILL.md.dev/features/pipeline-integration-probe/PLAN.md.dev/features/pipeline-integration-probe/REGRESSION.md.dev/features/pipeline-integration-probe/REVIEW.md.dev/features/pipeline-integration-probe/VERIFY.md.dev/features/pipeline-integration-probe/regression-report.json.dev/features/pipeline-integration-probe/verify-report.json.dev/features/reframe/PLAN.md.dev/features/reframe/REVIEW.md.dev/features/regress/PLAN.md.dev/features/regress/REVIEW.md.dev/features/revert-exit-label/PLAN.md.dev/features/review-scope-tighten/PLAN.md.dev/features/review-scope-tighten/REVIEW.md.dev/features/scope-setter-tighten/PLAN.md.dev/features/scope-setter-tighten/REVIEW.md.dev/features/ship-gated/PLAN.md.dev/features/ship-gated/REGRESSION.md.dev/features/ship-gated/REVIEW.md.dev/features/ship-gated/VERIFY.md.dev/features/ship-gated/regression-report.json.dev/features/ship-gated/verify-report.json.dev/features/ship-loop/PLAN.md.dev/features/ship-loop/REGRESSION.md.dev/features/ship-loop/REVIEW.md.dev/features/ship-loop/VERIFY.md.dev/features/ship-loop/regression-report.json.dev/features/ship-loop/verify-report.json.dev/features/structural-checker/PLAN.md.dev/features/structural-checker/REVIEW.md.dev/features/structured-findings/PLAN.md.dev/features/structured-findings/REVIEW.md.dev/features/trust-fence-baseline/PLAN.md.dev/features/trust-fence-baseline/REVIEW.md.dev/features/trust-fence-cite-action-line/PLAN.md.dev/features/trust-fence-cite-action-line/REVIEW.md.dev/features/trust-fence/NOTES.md.dev/features/trust-fence/PLAN.md.dev/features/trust-fence/REVIEW.md.dev/features/trust-fence/findings.json.dev/features/verifier-membership-frontmatter/PLAN.md.dev/features/verifier-membership-frontmatter/REVIEW.md.dev/features/verifier-membership-frontmatter/VERIFY.md.dev/features/verifier-membership-frontmatter/verify-report.json.dev/features/verify/PLAN.md.dev/features/verify/REVIEW.md.dev/features/writes-scope/PLAN.md.dev/features/writes-scope/REVIEW.md.dev/floor/README.md.dev/floor/check-provenance.mjs.dev/floor/check-provenance.test.mjs.dev/floor/check-regress.mjs.dev/floor/check-regress.test.mjs.dev/floor/check-ship.mjs.dev/floor/check-ship.test.mjs.dev/floor/check-structural.mjs.dev/floor/check-structural.test.mjs.dev/floor/check-variance.mjs.dev/floor/check-variance.test.mjs.dev/floor/check-verify.mjs.dev/floor/check-verify.test.mjs.dev/floor/count-verifiers.mjs.dev/floor/count-verifiers.test.mjs.dev/floor/test-fixtures/green/evals/cases/case-1.md.dev/floor/test-fixtures/green/evals/expected/expected-1.md.dev/floor/test-fixtures/green/skill.md.dev/floor/test-fixtures/red/skill.md.dev/floor/test-fixtures/structural/green.actual.json.dev/floor/test-fixtures/structural/green.expected.json.dev/floor/test-fixtures/structural/red-field-equals.actual.json.dev/floor/test-fixtures/structural/red-field-equals.expected.json.dev/floor/test-fixtures/structural/red-file-resolves.actual.json.dev/floor/test-fixtures/structural/red-file-resolves.expected.json.dev/floor/test-fixtures/structural/red-finding-count.actual.json.dev/floor/test-fixtures/structural/red-finding-count.expected.json.dev/floor/test-fixtures/structural/red-needle-present.actual.json.dev/floor/test-fixtures/structural/red-needle-present.expected.json.dev/floor/test-fixtures/structural/red-skill-kind.actual.json.dev/floor/test-fixtures/structural/red-skill-kind.expected.json.dev/floor/test-fixtures/variance/expected.json.dev/floor/test-fixtures/variance/finding-clean.json.dev/floor/test-fixtures/variance/finding-laundered.json.dev/floor/test-fixtures/variance/semantic-fail.json.dev/floor/test-fixtures/variance/semantic-pass.json.dev/floor/validate.mjs.dev/floor/validate.test.mjs.dev/memory-bank/feature-catalog.md.dev/memory-bank/lessons-learned.md.github/workflows/ci.yml.github/workflows/floor.yml.github/workflows/gitleaks.yml.markdownlint-cli2.jsonc.prettierignoreCLAUDE.mdCONTRIBUTING.mdREADME.mdeslint.config.mjsfeatures/README.mdfloor/validate.test.mjspackage.json
💤 Files with no reviewable changes (1)
- floor/validate.test.mjs
Comment on lines
+90
to
+93
| 3. **`/pharn-dev-build`** → writes the planned files and runs the floor. **Verdict read (FLOOR):** the exit code | ||
| of `node .dev/floor/validate.mjs .` — `0` (GREEN) → proceed; **non-zero** → **STOP**, present the RED | ||
| floor, hand to the human. (`/pharn-dev-build` itself HALTs on a RED floor and emits **no** machine report, so | ||
| the floor exit **is** its verdict — `ARCHITECTURE.md §2` primitive #3.) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Re-run /pharn-dev-build after every loop fix.
After a CONTINUE, the spec has the agent edit files and jump straight to /pharn-dev-regress → /pharn-dev-verify → /pharn-dev-review. That bypasses the build-stage floor on iterations 2+, even though this same file defines /pharn-dev-build as the stage that re-runs validate and the spec-hash check. A later iteration can therefore reach STOP_GREEN on code that never passed build's own floor.
Proposed fix
-1. **Iteration 1** = the gated `/pharn-dev-build → /pharn-dev-regress → /pharn-dev-verify → /pharn-dev-review` (after GATE 1).
+1. **Each iteration** runs `/pharn-dev-build → /pharn-dev-regress → /pharn-dev-verify → /pharn-dev-review` (after GATE 1 for iteration 1).
@@
- Then apply a **fix** to the failing gate **within the approved plan's `## Files`** (fix `#7` now pins
- it again — a write outside `## Files` is denied; never bypass the hook), and re-run
- `/pharn-dev-regress → /pharn-dev-verify → /pharn-dev-review`, `iter++`, and re-read the stop.
+ Then apply a **fix** to the failing gate **within the approved plan's `## Files`** (fix `#7` now pins
+ it again — a write outside `## Files` is denied; never bypass the hook), and re-run
+ `/pharn-dev-build → /pharn-dev-regress → /pharn-dev-verify → /pharn-dev-review`, `iter++`, and re-read the stop.Also applies to: 167-191, 201-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/pharn-dev-ship.md around lines 90 - 93, The loop flow in
/pharn-dev-ship skips the /pharn-dev-build floor after a CONTINUE, so later
iterations can reach /pharn-dev-regress and /pharn-dev-verify without re-running
the build-stage checks. Update the state-machine steps in this spec so every
loop fix routes back through /pharn-dev-build before regress/verify/review,
preserving the validate/spec-hash gate defined by /pharn-dev-build and the
floor-verdict behavior described in this document. Use the existing
/pharn-dev-build, /pharn-dev-regress, /pharn-dev-verify, /pharn-dev-review, and
STOP_GREEN flow references to keep the sequence consistent.
Comment on lines
+26
to
+31
| // sensitive zones (.dev/memory-bank/, .dev/floor/, .claude/, root files) are intentionally absent — | ||
| // reaching them requires an explicit `writes:` declaration. `.dev/features/**` (build-loop artifacts) | ||
| // IS in the set: the dev/product move relocated `features/**` there, and it keeps its prior | ||
| // writable-by-default behavior — every sensitive zone above still stays deny-by-default (it matches | ||
| // none of these globs). | ||
| const DEFAULT_SAFE_SET = ["features/**", ".dev/features/**", "pharn-*/**"]; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
DEFAULT_SAFE_SET still reopens root features/** when scope setup fails.
set-writes-scope.test.cjs now locks pharn-dev-* to .dev/features/**, but this fallback still allows features/** whenever .pharn/writes-scope.json is missing or unreadable. That means the new dev/product split is only enforced on the happy path; the fail-closed path still authorizes writes into the product features/ tree.
Suggested fix
-const DEFAULT_SAFE_SET = ["features/**", ".dev/features/**", "pharn-*/**"];
+const DEFAULT_SAFE_SET = [".dev/features/**", "pharn-*/**"];If root features/** still needs to stay writable for a separate product flow, that path should be granted explicitly by that flow rather than left in the generic fallback.
📝 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
| // sensitive zones (.dev/memory-bank/, .dev/floor/, .claude/, root files) are intentionally absent — | |
| // reaching them requires an explicit `writes:` declaration. `.dev/features/**` (build-loop artifacts) | |
| // IS in the set: the dev/product move relocated `features/**` there, and it keeps its prior | |
| // writable-by-default behavior — every sensitive zone above still stays deny-by-default (it matches | |
| // none of these globs). | |
| const DEFAULT_SAFE_SET = ["features/**", ".dev/features/**", "pharn-*/**"]; | |
| // sensitive zones (.dev/memory-bank/, .dev/floor/, .claude/, root files) are intentionally absent — | |
| // reaching them requires an explicit `writes:` declaration. `.dev/features/**` (build-loop artifacts) | |
| // IS in the set: the dev/product move relocated `features/**` there, and it keeps its prior | |
| // writable-by-default behavior — every sensitive zone above still stays deny-by-default (it matches | |
| // none of these globs). | |
| const DEFAULT_SAFE_SET = [".dev/features/**", "pharn-*/**"]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/hooks/enforce-writes-scope.cjs around lines 26 - 31, The fallback
allowlist in DEFAULT_SAFE_SET still grants write access to root features/** when
writes-scope setup fails, which bypasses the intended dev/product split. Update
the enforce-writes-scope.cjs scope defaults so the generic fallback is
fail-closed for product features and only keeps .dev/features/** (and other
truly safe globs) writable by default; if root features/** must remain writable,
grant it explicitly in the separate product flow rather than through
DEFAULT_SAFE_SET.
Comment on lines
+99
to
+129
| function main() { | ||
| const argv = process.argv.slice(2); | ||
| // Leading positionals = everything before the first `--flag` (so a flag VALUE like `--iter 2` can never | ||
| // leak in as a report path). The command always passes the two report files first, then the flags. | ||
| const positional = []; | ||
| for (const a of argv) { | ||
| if (a.startsWith("--")) break; | ||
| positional.push(a); | ||
| } | ||
|
|
||
| const verify = readVerdict(positional[0], "verify-report.json", VERIFY_VERDICTS); | ||
| const regress = readVerdict(positional[1], "regression-report.json", REGRESS_VERDICTS); | ||
| const iterR = posInt(flag(argv, "--iter"), "iter"); | ||
| const capR = posInt(flag(argv, "--cap"), "cap"); | ||
|
|
||
| // Fail-closed (P5): any malformed operand → INCONCLUSIVE (exit 2), NEVER a silent CONTINUE. Echo back | ||
| // whatever parsed cleanly (nulls otherwise) plus the helper's OWN diagnostic `reason` (not free-text). | ||
| const bad = [verify, regress, iterR, capR].find((r) => !r.ok); | ||
| if (bad) { | ||
| emit( | ||
| { | ||
| verify_verdict: verify.ok ? verify.verdict : null, | ||
| regress_verdict: regress.ok ? regress.verdict : null, | ||
| floor_green: null, | ||
| iter: iterR.ok ? iterR.value : null, | ||
| cap: capR.ok ? capR.value : null, | ||
| decision: "INCONCLUSIVE", | ||
| reason: bad.reason, | ||
| }, | ||
| 2 | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unexpected argv instead of ignoring it.
This parser still returns STOP_*/CONTINUE when the invocation shape is wrong, because extra positionals, unknown flags, and duplicate --iter/--cap flags are silently ignored. That breaks the helper's own fail-closed contract for bad input.
Proposed fix
function main() {
const argv = process.argv.slice(2);
@@
const positional = [];
for (const a of argv) {
if (a.startsWith("--")) break;
positional.push(a);
}
+ const flags = argv.filter((a) => a.startsWith("--"));
+ if (
+ positional.length !== 2 ||
+ flags.some((a) => a !== "--iter" && a !== "--cap") ||
+ flags.filter((a) => a === "--iter").length !== 1 ||
+ flags.filter((a) => a === "--cap").length !== 1
+ ) {
+ emit(
+ {
+ verify_verdict: null,
+ regress_verdict: null,
+ floor_green: null,
+ iter: null,
+ cap: null,
+ decision: "INCONCLUSIVE",
+ reason: "expected exactly two report paths plus one --iter and one --cap flag",
+ },
+ 2
+ );
+ }
const verify = readVerdict(positional[0], "verify-report.json", VERIFY_VERDICTS);
const regress = readVerdict(positional[1], "regression-report.json", REGRESS_VERDICTS);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dev/floor/check-ship.mjs around lines 99 - 129, The argv parsing in main()
is still too permissive because extra positionals, unknown flags, and duplicate
--iter/--cap occurrences can be ignored, allowing STOP_* or CONTINUE instead of
fail-closed behavior. Tighten the parser around argv, positional, flag(),
posInt(), and emit() so any unexpected argument shape is treated as invalid
input: reject extra non-flag args, reject unrecognized flags, and reject
repeated known flags before verdict evaluation. Make the malformed-invocation
path return the same INCONCLUSIVE/exit-2 handling used for other bad operands,
rather than silently continuing.
Comment on lines
+16
to
+20
| // Usage: node .dev/floor/validate.mjs [targetDir] (default: cwd) | ||
| // Honest scope: checks 5 and 6 are BEST-EFFORT — markdown has no import statement to lint, so they | ||
| // reduce a class of mistakes, they do not eliminate it (see ARCHITECTURE §4 caveat, LIMITS). | ||
| // | ||
| // It deliberately does NOT validate this repo's own tooling (.claude/commands, floor/) — those | ||
| // It deliberately does NOT validate this repo's own tooling (.claude/commands, .dev/) — those |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce repository-root validation instead of allowing arbitrary targetDir.
This still lets callers validate subtrees instead of the product surface. If someone runs node .dev/floor/validate.mjs .dev, relative(TARGET, file) strips the .dev/ prefix and isExcluded() stops excluding tooling files, so .dev/ content can be counted by the floor.
As per coding guidelines, ".dev/floor/validate.mjs: The floor validator must treat the repository root as the product surface and ignore the repository's own tooling directories such as .claude/commands/ and .dev/."
Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dev/floor/validate.mjs around lines 16 - 20, The floor validator currently
accepts an arbitrary targetDir, which can let `.dev/` or other subtrees be
counted as product surface; update `validate.mjs` so it always validates from
the repository root and keeps the tooling exclusions intact. Adjust the
`TARGET`/path resolution and any related checks in the validator flow
(`relative(TARGET, file)`, `isExcluded()`, and the main scan logic) so
`.claude/commands/` and `.dev/` are always ignored regardless of the caller’s
argument.
Source: Coding guidelines
…omote L8 Introduce the first product pipeline command with deterministic SPEC.md shape/state/hash validation, dogfood build artifacts, and the writes-scope setter lesson from design review. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/commands/pharn-spec.md:
- Line 2: The command description has a lifecycle mismatch for spec_id: it
currently says spec_id is assigned only on approval, but the Draft output and
check-spec.mjs require spec_id to exist before approval. Update the pharn-spec
prompt text so SPEC.md always includes spec_id from the Draft stage onward, and
reserve approval only for flipping state from Draft to Approved and setting
spec_content_hash. Keep the wording aligned with the Draft/Approved flow in the
command description so it no longer implies spec_id is created only after
approval.
In @.dev/floor/check-spec.mjs:
- Around line 103-109: The emitHash() path is swallowing unreadable input errors
because it returns 1 immediately after readText() fails, before fail() can print
the collected diagnostics. Update emitHash() in check-spec.mjs to surface the
unreadable SPEC.md error in --hash mode by calling fail() (or otherwise emitting
the accumulated reds[] diagnostics) when readText() returns undefined, keeping
the existing parseSpec() and error handling flow intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40969477-a267-466b-b97c-967e2d47fcab
📒 Files selected for processing (10)
.claude/commands/pharn-spec.md.dev/features/pharn-spec/PLAN.md.dev/features/pharn-spec/REGRESSION.md.dev/features/pharn-spec/REVIEW.md.dev/features/pharn-spec/VERIFY.md.dev/features/pharn-spec/regression-report.json.dev/features/pharn-spec/verify-report.json.dev/floor/check-spec.mjs.dev/floor/check-spec.test.mjs.dev/memory-bank/lessons-learned.md
✅ Files skipped from review due to trivial changes (5)
- .dev/features/pharn-spec/verify-report.json
- .dev/features/pharn-spec/regression-report.json
- .dev/features/pharn-spec/VERIFY.md
- .dev/memory-bank/lessons-learned.md
- .dev/features/pharn-spec/REVIEW.md
| @@ -0,0 +1,203 @@ | |||
| --- | |||
| description: "Turn a user's prose intent into a structured, human-approved features/<name>/SPEC.md — the head of the product pipeline (spec → plan → grill → build → regress → verify → ship) and the versioned record of INTENT every downstream stage reads. INTERROGATES the intent for gaps (advisory — never gates), EMITS a Draft SPEC.md with required sections, then HALTS for explicit human approval; only on approval does it flip Draft → Approved, assign a spec_id, and pin the approved intent with a content-hash (fix #4). FLOOR (deterministic, .dev/floor/check-spec.mjs): required-section PRESENCE, the Draft|Approved state enum, spec_id presence, and — when Approved — spec_content_hash == sha256(body). ADVISORY/HUMAN: whether the intent is clear/complete/wise — the human owns that, and owns the Draft → Approved gate. The model NEVER self-approves. '/pharn-spec produced it' NEVER means 'the intent is sound' (P0)." | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the spec_id lifecycle consistent with the checker.
The description says spec_id is assigned only on approval, but Step 3 in this same command writes spec_id: <name> into the Draft, and .dev/floor/check-spec.mjs rejects Draft specs without spec_id. Leaving this contradiction at the top of the prompt can steer the agent into producing a RED draft. Approval should only flip state and pin spec_content_hash; spec_id needs to exist from Draft onward.
Proposed fix
-description: "Turn a user's prose intent into a structured, human-approved features/<name>/SPEC.md — the head of the product pipeline (spec → plan → grill → build → regress → verify → ship) and the versioned record of INTENT every downstream stage reads. INTERROGATES the intent for gaps (advisory — never gates), EMITS a Draft SPEC.md with required sections, then HALTS for explicit human approval; only on approval does it flip Draft → Approved, assign a spec_id, and pin the approved intent with a content-hash (fix `#4`). FLOOR (deterministic, .dev/floor/check-spec.mjs): required-section PRESENCE, the Draft|Approved state enum, spec_id presence, and — when Approved — spec_content_hash == sha256(body). ADVISORY/HUMAN: whether the intent is clear/complete/wise — the human owns that, and owns the Draft → Approved gate. The model NEVER self-approves. '/pharn-spec produced it' NEVER means 'the intent is sound' (P0)."
+description: "Turn a user's prose intent into a structured, human-approved features/<name>/SPEC.md — the head of the product pipeline (spec → plan → grill → build → regress → verify → ship) and the versioned record of INTENT every downstream stage reads. INTERROGATES the intent for gaps (advisory — never gates), EMITS a Draft SPEC.md with `spec_id: <name>` and the required sections, then HALTS for explicit human approval; only on approval does it flip Draft → Approved and pin the approved intent with a content-hash (fix `#4`). FLOOR (deterministic, .dev/floor/check-spec.mjs): required-section PRESENCE, the Draft|Approved state enum, spec_id presence, and — when Approved — spec_content_hash == sha256(body). ADVISORY/HUMAN: whether the intent is clear/complete/wise — the human owns that, and owns the Draft → Approved gate. The model NEVER self-approves. '/pharn-spec produced it' NEVER means 'the intent is sound' (P0)."📝 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
| description: "Turn a user's prose intent into a structured, human-approved features/<name>/SPEC.md — the head of the product pipeline (spec → plan → grill → build → regress → verify → ship) and the versioned record of INTENT every downstream stage reads. INTERROGATES the intent for gaps (advisory — never gates), EMITS a Draft SPEC.md with required sections, then HALTS for explicit human approval; only on approval does it flip Draft → Approved, assign a spec_id, and pin the approved intent with a content-hash (fix #4). FLOOR (deterministic, .dev/floor/check-spec.mjs): required-section PRESENCE, the Draft|Approved state enum, spec_id presence, and — when Approved — spec_content_hash == sha256(body). ADVISORY/HUMAN: whether the intent is clear/complete/wise — the human owns that, and owns the Draft → Approved gate. The model NEVER self-approves. '/pharn-spec produced it' NEVER means 'the intent is sound' (P0)." | |
| description: "Turn a user's prose intent into a structured, human-approved features/<name>/SPEC.md — the head of the product pipeline (spec → plan → grill → build → regress → verify → ship) and the versioned record of INTENT every downstream stage reads. INTERROGATES the intent for gaps (advisory — never gates), EMITS a Draft SPEC.md with `spec_id: <name>` and the required sections, then HALTS for explicit human approval; only on approval does it flip Draft → Approved and pin the approved intent with a content-hash (fix `#4`). FLOOR (deterministic, .dev/floor/check-spec.mjs): required-section PRESENCE, the Draft|Approved state enum, spec_id presence, and — when Approved — spec_content_hash == sha256(body). ADVISORY/HUMAN: whether the intent is clear/complete/wise — the human owns that, and owns the Draft → Approved gate. The model NEVER self-approves. '/pharn-spec produced it' NEVER means 'the intent is sound' (P0)." |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/pharn-spec.md at line 2, The command description has a
lifecycle mismatch for spec_id: it currently says spec_id is assigned only on
approval, but the Draft output and check-spec.mjs require spec_id to exist
before approval. Update the pharn-spec prompt text so SPEC.md always includes
spec_id from the Draft stage onward, and reserve approval only for flipping
state from Draft to Approved and setting spec_content_hash. Keep the wording
aligned with the Draft/Approved flow in the command description so it no longer
implies spec_id is created only after approval.
Comment on lines
+103
to
+109
| function emitHash(specPath) { | ||
| const text = readText(specPath, "SPEC.md"); | ||
| if (text === undefined) return 1; | ||
| const parsed = parseSpec(text); | ||
| if (!parsed) { | ||
| console.error(`check-spec: no YAML frontmatter in ${specPath} — cannot locate the body to hash`); | ||
| return 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Print the unreadable-input error in --hash mode.
This path exits 1 without any diagnostic: readText() only appends to reds[], and emitHash() returns before fail() is called. A missing/unreadable SPEC.md then looks like a silent pinning failure.
Proposed fix
function emitHash(specPath) {
const text = readText(specPath, "SPEC.md");
- if (text === undefined) return 1;
+ if (text === undefined) return fail();
const parsed = parseSpec(text);
if (!parsed) {
console.error(`check-spec: no YAML frontmatter in ${specPath} — cannot locate the body to hash`);
return 1;
}📝 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
| function emitHash(specPath) { | |
| const text = readText(specPath, "SPEC.md"); | |
| if (text === undefined) return 1; | |
| const parsed = parseSpec(text); | |
| if (!parsed) { | |
| console.error(`check-spec: no YAML frontmatter in ${specPath} — cannot locate the body to hash`); | |
| return 1; | |
| function emitHash(specPath) { | |
| const text = readText(specPath, "SPEC.md"); | |
| if (text === undefined) return fail(); | |
| const parsed = parseSpec(text); | |
| if (!parsed) { | |
| console.error(`check-spec: no YAML frontmatter in ${specPath} — cannot locate the body to hash`); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dev/floor/check-spec.mjs around lines 103 - 109, The emitHash() path is
swallowing unreadable input errors because it returns 1 immediately after
readText() fails, before fail() can print the collected diagnostics. Update
emitHash() in check-spec.mjs to surface the unreadable SPEC.md error in --hash
mode by calling fail() (or otherwise emitting the accumulated reds[]
diagnostics) when readText() returns undefined, keeping the existing parseSpec()
and error handling flow intact.
…ship reads: - pharn-dev-ship.md: add .dev/features/<name>/PLAN.md to reads: (used by the slug + set-writes-scope --from-plan in --loop mode) - check-ship.mjs: strict, fail-closed argv parse — reject extra positionals, unrecognized flags, and repeated --iter/--cap → INCONCLUSIVE (exit 2) instead of a silent STOP_*/CONTINUE; add hermetic tests for each rejection path - dev-product-boundary/PLAN.md: make the no-stale-reference backstop grep use a real path pattern (features/[a-z0-9-]+/) instead of the literal <name> token Co-authored-by: Cursor <cursoragent@cursor.com>
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
/pharn-dev-shipas a gated pipeline orchestrator (plan → grill → build → regress → verify → review) with floor-backedcheck-ship.mjsand green regression/verify artifacts./pharn-dev-shipwith--loopmode and a tested stop core incheck-ship.mjsfor iterative ship runs.floor/, build-loopfeatures/, andmemory-bank/under.dev/; renames the nine build commands to thepharn-dev-prefix; creates rootfeatures/for product-pipeline artifacts; updates floor to exclude.dev/wholesale (zero behavior change on the product surface).Test plan
npm test— hook + floor suites greennpm run check— format, lint, markdownlint, testsnode .dev/floor/validate.mjs .— product surface still GREEN.dev/floor/paths/pharn-dev-shipand/pharn-dev-ship --loopcommand docs resolve under.claude/commands/pharn-dev-*.mdMade with Cursor
Summary by CodeRabbit
/pharn-dev-shiporchestrator (including--loop) with deterministic floor-based stop/continue behavior..dev/features/build-loop artifact structure and related ship-gated/ship-loop specs..dev/features/**and rejecting rootfeatures/targets..dev/pathing and naming across dev command docs and flows (pharn-dev-*)..dev/**floor checks and updated fixtures/paths.