From 7db88b98814c622b61fdfb5c73def8b2df664582 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Acharn=C3=A9?= Date: Sun, 2 Aug 2026 16:32:39 +0700 Subject: [PATCH 1/2] fix(orchestration): harden promotion review safeguards --- .../002-complete-execution-orchestration.md | 19 ++++++++++-- .../src/check-execution-orchestration.mjs | 8 ++++- .../test/execution-orchestration.test.mjs | 29 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/docs/plans/002-complete-execution-orchestration.md b/docs/plans/002-complete-execution-orchestration.md index a63e424f..fd7cc544 100644 --- a/docs/plans/002-complete-execution-orchestration.md +++ b/docs/plans/002-complete-execution-orchestration.md @@ -47,6 +47,14 @@ Code is evidence of work, not evidence of full requirement completion. Use these | `released` | A coordinated signed release contains the verified requirement. | A later version; never silently reverted in the ledger | | `blocked` | External authority or state is required after safe alternatives are exhausted. | `planned` or `partial` after the blocker is resolved | +The ledger also uses these plan/task states; they are not requirement statuses and must not be copied into a requirement record: + +| Plan/task state | Meaning | Allowed next state | +|---|---|---| +| `partial-needs-reconciliation` | Existing code or historical evidence exists, but the current checkpoint still needs an explicit reconciliation task. | `in-progress`, `planned`, or `blocked` | +| `in-progress` | The selected task is actively being delivered on one branch/worktree. | `implemented`, `verified`, or `blocked` | +| `post-ga-planned` | An opt-in P2 plan is intentionally held until GA release. | `in-progress` after GA, or `blocked` | + Never infer `verified` from a merged PR, a green unit test, file existence, or a previous model's prose. ## 2. Recorded checkpoint @@ -95,7 +103,7 @@ Parallel execution is allowed only when all of these are true: - One integration owner resolves contract/migration ordering before either branch merges. - The combined `dev` to `main` promotion remains below 280 changed files, leaving margin under CodeRabbit's 300-file limit. -Safe parallel lanes after dogfood are FA→SA, QI, and OC. CRF and PDA may overlap only after their shared renderer/semantic interfaces are frozen. MR, DQG, and EI may overlap after a migration-number and contract-version reservation is recorded. The foundation spine 010→070 stays serial. +Safe parallel lanes after dogfood are FA→SA, QI, and OC. CRF/PDA and MR/DQG remain serial by default. EI may overlap with those later plans only when both control records declare the matching interface-level entry gates, contract/migration reservations, and integration owner; otherwise they remain serial too. The foundation spine 010→070 stays serial. ## 4. Repository path contract @@ -119,7 +127,14 @@ Every feature uses these exact roots; use the module key in the final column rat | 310 | `services/api/src/features/dqg` and `services/api/prisma/schema/dqg.prisma` | `data-quality-guard` | | 320 | `services/api/src/features/ei` and `services/api/prisma/schema/ei.prisma` | `embedded-importer` | -Feature clients live under `apps/web/src/features/`, `apps/desktop/src/features/`, and `apps/android/app/src/main/kotlin/com/databreeze/`. Deterministic processors live under `services/engine/src/databreeze_engine/processors/`. Canonical schemas live under `packages/contracts/schemas/v1/`; pure domain types live under `packages/domain/src//v1.ts`. Do not create aggregate prose-named modules such as `identity-audit-entitlements` or `production-readiness` in application code. +Feature clients live under `apps/web/src/features/` and `apps/desktop/src/features/`. Android package directories use the deterministic `android-key`; engine processor directories use the deterministic `python-key`: + +| Derived key | Transformation | Example | +|---|---|---| +| `android-key` | Start with the lowercase ASCII module key, replace separators (`-` and `_`) with boundaries, remove all non-alphanumeric characters, and require the first character to be a letter. | `folder-autopilot` → `folderautopilot`; `private-data-analyst` → `privatedataanalyst` | +| `python-key` | Start with the lowercase ASCII module key, replace every separator or invalid character with one underscore, collapse repeated underscores, and require the first character to be a letter. | `folder-autopilot` → `folder_autopilot`; `private-data-analyst` → `private_data_analyst` | + +Thus Android paths are `apps/android/app/src/main/kotlin/com/databreeze/` and deterministic processors are `services/engine/src/databreeze_engine/processors/`. Canonical schemas live under `packages/contracts/schemas/v1/`; pure domain types live under `packages/domain/src//v1.ts`. Do not create aggregate prose-named modules such as `identity-audit-entitlements` or `production-readiness` in application code. ## 5. Atomic task execution contract diff --git a/tools/repo-cli/src/check-execution-orchestration.mjs b/tools/repo-cli/src/check-execution-orchestration.mjs index f90f3761..15688116 100644 --- a/tools/repo-cli/src/check-execution-orchestration.mjs +++ b/tools/repo-cli/src/check-execution-orchestration.mjs @@ -86,7 +86,13 @@ function escapeRegExp(value) { function pathExists(repositoryRoot, declaredPath) { if (typeof declaredPath !== 'string' || declaredPath.trim() === '') return false; if (/[{}*?]/u.test(declaredPath)) return false; - return existsSync(path.join(repositoryRoot, ...declaredPath.split('/'))); + if (declaredPath.includes('\\')) return false; + const segments = declaredPath.split('/'); + if (segments.some((segment) => segment === '.' || segment === '..')) return false; + const candidate = path.resolve(repositoryRoot, ...segments); + const relative = path.relative(repositoryRoot, candidate); + if (relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative)) return false; + return existsSync(candidate); } function validateDag(plans, diagnostics) { diff --git a/tools/repo-cli/test/execution-orchestration.test.mjs b/tools/repo-cli/test/execution-orchestration.test.mjs index 827bf856..9eb2f267 100644 --- a/tools/repo-cli/test/execution-orchestration.test.mjs +++ b/tools/repo-cli/test/execution-orchestration.test.mjs @@ -186,6 +186,17 @@ test('ledger records verified task evidence before advancing the next task', () ); }); +test('CodeRabbit promotion disposition records one review and rejected claims', () => { + const disposition = readFileSync( + path.join(repositoryRoot, 'docs', 'operations', 'code-review-11-disposition.md'), + 'utf8', + ); + assert.match(disposition, /Promotion PR.*#11/u); + assert.match(disposition, /one permitted full CodeRabbit review/u); + assert.match(disposition, /Duplicate plan catalog/u); + assert.match(disposition, /Docstring coverage warning/u); +}); + test('repository checker rejects dependency cycles', () => { withTemporaryPlans( ({ ledger }) => { @@ -212,3 +223,21 @@ test('repository checker rejects false verified requirement evidence', () => { }, ); }); + +test('repository checker rejects task evidence paths that escape the repository root', () => { + withTemporaryPlans( + ({ ledger }) => { + ledger.taskState = { + 'FND-001': { + status: 'verified', + commit: '0'.repeat(40), + evidence: ['..'], + }, + }; + }, + (result) => { + assert.notEqual(result.status, 0); + assert.match(result.stderr, /verified task FND-001 has missing evidence paths/u); + }, + ); +}); From 0ea0e5f732b75c3be7ff0cc42cb85decacf5a074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Acharn=C3=A9?= Date: Sun, 2 Aug 2026 16:32:42 +0700 Subject: [PATCH 2/2] docs(review): record promotion review dispositions --- docs/operations/code-review-11-disposition.md | 40 +++++++++++++++++++ .../foundation-reconciliation-2026-08-02.md | 4 +- docs/plans/003-luna-handoff-runbook.md | 10 ++--- .../test/foundation-reconciliation.test.mjs | 5 ++- 4 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 docs/operations/code-review-11-disposition.md diff --git a/docs/operations/code-review-11-disposition.md b/docs/operations/code-review-11-disposition.md new file mode 100644 index 00000000..bd196ad9 --- /dev/null +++ b/docs/operations/code-review-11-disposition.md @@ -0,0 +1,40 @@ +# CodeRabbit Review 11 Disposition + +**Promotion PR:** [#11](https://github.com/DatabreezeService/databreeze-platform/pull/11) + +**Review run:** `8cc266ad-874b-4781-a97d-ebe86b0eb521` + +**Completed at (UTC):** 2026-08-02T09:28:06Z + +**Invocation policy:** This was the one permitted full CodeRabbit review for PR #11. No second review will be requested. + +## Valid issues fixed + +The following ten valid issues (nine inline comments plus one task-conditional nitpick) were reproduced against the reviewed `dev` commit and are being fixed in focused commits before promotion: + +1. The Luna bootstrap prompt now makes PostgreSQL migration/tenant tests conditional on durable-state changes and client coverage conditional on client behavior. +2. The evidence record stores the full source commit SHA while retaining the short display prefix. +3. The rollback note explains that reverting the reconciliation test removes `repo:check` enforcement and requires a fresh check. +4. Parallel-lane guidance now keeps CRF/PDA and MR/DQG serial by default and requires explicit interface-level control records before any overlap. +5. Android and Python normalized-key transformations are defined deterministically with examples. +6. The orchestration state section separates requirement states from plan/task states and includes all ledger vocabulary transitions. +7. The handoff record captures the CodeRabbit invocation timestamp in UTC. +8. Redis-loss recovery now fences stale workers and reconciles durable leases/effect receipts before redispatch. +9. Worktree instructions now select `feat/`, `fix/`, `docs/`, `ci/`, or another conventional prefix based on task type. +10. The orchestration path validator rejects backslashes, dot segments, parent segments, and resolved paths outside the repository root. + +The review grouped the first item as a nitpick and the remaining items as inline comments; all are recorded here because they affect the safety contract. + +## Rejected issues with evidence + +### Duplicate plan catalog + +CodeRabbit suggested moving the plan catalog out of `tools/repo-cli/src/check-execution-orchestration.mjs` and importing it from the test. The test intentionally keeps an independent expected catalog: it is the oracle that detects a checker or ledger silently dropping, reordering, or recounting a plan. Sharing the same map would allow the checker and test to drift together and would remove that protection. The current duplication is therefore deliberate and documented; no code change is made. + +### Docstring coverage warning + +The walkthrough reported a 0% docstring-coverage warning. Docstring coverage is not a repository check, release gate, or requirement in this project. The affected files are executable CLI/checker code and Markdown evidence, and adding decorative docstrings would not improve the validated behavior. The repository’s actual gates—format, lint, typecheck, contract parity, tests, builds, scans, Android checks, and infrastructure static checks—remain the acceptance evidence. + +## Verification + +Valid fixes must pass targeted tests, `corepack pnpm repo:check`, `corepack pnpm repo:build`, and the hosted checks on the follow-up `dev` PR. This document is evidence of the rejected comments and must remain in the promotion diff; it does not claim that a second CodeRabbit review occurred. diff --git a/docs/operations/foundation-reconciliation-2026-08-02.md b/docs/operations/foundation-reconciliation-2026-08-02.md index 8df7a6e0..0f393ff2 100644 --- a/docs/operations/foundation-reconciliation-2026-08-02.md +++ b/docs/operations/foundation-reconciliation-2026-08-02.md @@ -2,7 +2,7 @@ **Evidence date:** 2026-08-02 -**Source commit:** `86e72d8` +**Source commit:** `86e72d8569057d2a14ed6bb1672ce6a573fa8d7c` (display prefix: `86e72d8`) **Scope:** the merged engineering-foundation implementation and the 23 tasks in `docs/plans/010-engineering-foundation.md`. @@ -65,4 +65,4 @@ The repository checks include generated-contract drift, brand checksum/derivativ ## Release and rollback decision -FND-001 is complete as an evidence-reconciliation task. Plan 010 remains `partial-needs-reconciliation` until FND-002 through FND-007 close their independent gates and hosted OpenTofu validation is available. The next orchestration task is `FND-002`. Reverting this record and its test removes only reconciliation evidence; it does not alter application code, generated contracts, migrations, assets, or runtime state. +FND-001 is complete as an evidence-reconciliation task. Plan 010 remains `partial-needs-reconciliation` until FND-002 through FND-007 close their independent gates and hosted OpenTofu validation is available. The next orchestration task is `FND-002`. Reverting this record and its test removes the reconciliation evidence and its `repo:check` enforcement; after such a rollback, run `corepack pnpm repo:check` and record the resulting gap before merging. The rollback does not alter application code, generated contracts, migrations, assets, or runtime state. diff --git a/docs/plans/003-luna-handoff-runbook.md b/docs/plans/003-luna-handoff-runbook.md index 2de4795f..b9b3c579 100644 --- a/docs/plans/003-luna-handoff-runbook.md +++ b/docs/plans/003-luna-handoff-runbook.md @@ -20,11 +20,11 @@ Use this runbook to resume DataBreeze after a model, machine, branch, or hosted- 4. Run `corepack pnpm orchestration:check`. Treat `execution-orchestration.json.checkpoint` as historical only; recompute the live PR/branch state. 5. Inspect the selected task's requirement records. A record marked `implemented`, `verified`, or `released` must have real code/test/evidence paths that exist and match the current commit. Downgrade an unsupported status in the same corrective commit; never preserve a false completion claim. -6. If a clean checkout is required, create an ignored worktree from the current integration base. Never reuse a worktree with unrelated user changes: +6. If a clean checkout is required, create an ignored worktree from the current integration base. Select the branch prefix from the task type (`feat` for capability work, `fix` for corrections, `docs` for documentation, `ci` for workflow-only work, or another conventional prefix recorded in the task). Never reuse a worktree with unrelated user changes: ```powershell git check-ignore -q .worktrees - git worktree add .worktrees/ -b feat/ origin/dev + git worktree add .worktrees/ -b / origin/dev ``` 7. Bootstrap exactly as repository documentation specifies. The known clean-checkout sequence is: @@ -115,7 +115,7 @@ For each `#### TASK-ID —` item in `002-complete-execution-orchestration.md`: | Duplicate command/job/webhook/client mutation | Existing idempotency/effect receipt matches | Return the prior outcome; never repeat a consequential effect | | Same idempotency key has different payload | Stored request hash differs | Return a stable conflict/security problem and audit it; do not choose either silently | | Lease expires while a worker finishes | Attempt/lease revision is stale | Reject/quarantine the result, clean grants/temp state, and let authoritative scheduling decide retry | -| Redis is lost | Dispatch/cache/lock disappears | Reconstruct from PostgreSQL outbox/jobs; Redis is never authority | +| Redis is lost | Dispatch/cache/lock disappears | First reconcile durable attempts and leases, fence stale workers, and verify idempotency/effect receipts; then rebuild dispatch hints from PostgreSQL outbox/jobs and redispatch only eligible work. Redis is never authority. | | Object store is partially available | Multipart/grant/hash operation fails | Keep state resumable, avoid finalization until verification, expire grants, and reconcile abandoned parts | | Database migration fails halfway | Migration journal/verify stage fails | Stop deploy, use the rehearsed compatible rollback/compensation path, preserve immutable records, and restore only from verified recovery points | | Contract generation differs by runtime | Drift/parity check fails | Fix canonical schema/generator/version, regenerate all runtimes, and block merge | @@ -143,7 +143,7 @@ Canonical repository/worktree: Branch / HEAD / upstream: Remote dev / main: Open feature PR / promotion PR: -CodeRabbit invocation count and review URL: +CodeRabbit invocation count, invocation timestamp (UTC), and review URL: Active plan / task ID: Requirement IDs and statuses changed: Completed commits (hash — outcome): @@ -169,7 +169,7 @@ You are resuming DataBreeze in the canonical databreeze-platform repository. Do Live verified checkpoint: branch [BRANCH], HEAD [HEAD], origin/dev [DEV], origin/main [MAIN], open feature PR [FEATURE_PR_OR_NONE], open dev→main promotion PR [PROMOTION_PR_OR_NONE]. Run the orchestration checker and the documented clean baseline before edits. Preserve all user changes and use an ignored worktree if isolation is needed. -Resume task [TASK_ID] only after proving its dependency/entry gate. Follow test-first atomic delivery: canonical contracts, failing domain tests, real PostgreSQL migration/tenant tests, implementation through ports, vertical client/adapter coverage, safe telemetry/recovery, traceability evidence, scoped checks, repo:check, repo:build, diff review, and one reversible commit. Do not mark merged code verified without all evidence. +Resume task [TASK_ID] only after proving its dependency/entry gate. Follow test-first atomic delivery: canonical contracts when the interface changes, failing domain/state tests, PostgreSQL migration/tenant/transaction/outbox tests when durable state changes, implementation through ports, vertical client/adapter coverage when the task involves client behavior, safe telemetry/recovery, traceability evidence, scoped checks, repo:check, repo:build, diff review, and one reversible commit. For documentation-only or other non-durable/non-client tasks, record why those conditional tests do not apply. Do not mark merged code verified without all evidence. Git flow is fixed: feat/* or fix/* → PR to dev with hosted checks and no CodeRabbit; merge preserving atomic commits; immediately open dev→main; request exactly one CodeRabbit full review there; reproduce every comment, fix only valid findings, document rejected ones, never request a second review on that PR. Prefer 30–50 commits, hard cap 60, and do not invoke the promotion review over 280 changed files. diff --git a/tools/repo-cli/test/foundation-reconciliation.test.mjs b/tools/repo-cli/test/foundation-reconciliation.test.mjs index 69b38edb..20898e70 100644 --- a/tools/repo-cli/test/foundation-reconciliation.test.mjs +++ b/tools/repo-cli/test/foundation-reconciliation.test.mjs @@ -17,7 +17,10 @@ test('foundation reconciliation records every approved foundation task and gate' const evidence = readFileSync(evidencePath, 'utf8'); assert.match(evidence, /^# Engineering Foundation Reconciliation$/mu); - assert.match(evidence, /\*\*Source commit:\*\* `86e72d8`/u); + assert.match( + evidence, + /\*\*Source commit:\*\* `86e72d8569057d2a14ed6bb1672ce6a573fa8d7c` \(display prefix: `86e72d8`\)/u, + ); assert.match(evidence, /\*\*Requirement status:\*\* no requirement promoted to `verified`/u); for (let taskNumber = 1; taskNumber <= 23; taskNumber += 1) { assert.match(evidence, new RegExp(`\\| Task ${taskNumber} \\|`, 'u'));