ci(workflow): make the required gate execute and verify what it ships (Wave 0) - #80
Merged
Conversation
Wave 0 of the Phase 2.5.5/2.6 execution order — the CI-truth batch. Every item below is one root cause: a guard that claimed a stronger guarantee than it enforced. Landed as one change because five of them edit the same file family and splitting them would leave the gate self-inconsistent in between. * The required `ci` job never ran the binary it built (#294). Only the advisory Windows leg and the tag-gated release smoke ever executed `apps/cli/dist/index.js`, so a bundle that compiled but could not boot merged green. Adds a `--version` / `--help` / fixture `run --json` smoke. * `apps/cli/drizzle/**` was not a declared turbo build output (#315), although `tsup`'s `onSuccess` copies the migrations there, outside `dist/`. A cache-hit replay could therefore leave `dist/index.js` fresh beside a stale or missing `drizzle/`, crashing on first DB touch with nothing red anywhere. Verified by deleting the directory and replaying from cache: 14 files restored after the fix, none before. Declared in a new `apps/cli/turbo.json` extending the root. * `pnpm ci` and `ci.yml` had diverged in both directions (#312): the root script ran `lint:tools`, which CI never did; CI ran a DB-migration-sync check the root script never did. Adds the missing step to CI and a `db:sync-check` script to the root, mirroring CI's `git status --porcelain` (which, unlike `git diff`, also catches an untracked new migration). * The >=90% engine coverage floor was documented as enforced and configured as advisory (#296, #152). Resolved by ruling rather than by wording: `llm` and `mcp` become a REQUIRED check; `core` stays measured-and-printed but non-blocking, because its branch margin is +0.83 (90.83, measured 2026-07-29) and Waves 1-3 edit `core` heavily. `RELAVIUM_COVERAGE_ENFORCED_ONLY=1` selects the CI subset; a local `pnpm coverage` still enforces all three, so the floor never silently relaxes for a developer. Behaviourally verified: with `core`'s floor raised to 99, `coverage:enforced` passes and `coverage` fails. * `floor-check` and `peer-dep-gate` were advisory without saying so (#320) — the `name:` is what the PR checks list shows, so both now carry `(advisory)`. * `THIRD_PARTY_EXTERNAL` was stale versus `package.json` (G27, #248): `string-width` shipped with ADR-0069 and was never added. Harmless today only because tsup auto-externalizes anything already in `dependencies`. * The bundle-closure guard read one output chunk without asserting there is one (#314). Correct today, silently narrowing the moment a dynamic `import()` splits a chunk — which 2.5.5.E's `driveHome` deferral will do deliberately. * `sync:models:check` was implemented, documented as the CI-facing staleness mode, and called by nothing (#317). Wired into the weekly job as `continue-on-error` so it reports drift without paging on ordinary churn. `testing.md` is corrected in the same change rather than later: the standard previously asserted an enforcement the pipeline did not perform, and stating the split only in `ci.yml` would have left that contradiction in the standard. Refs Phase 2.5.5 sub-stream H. G26 (release-ref ancestry) already landed in #77. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer's GuideAligns the required CI gate with what Sequence diagram for the required CI job building and smoking the CLI binarysequenceDiagram
participant GitHubActions_ci as GitHubActions_ci
participant Turbo as turbo_run
participant DB as db_sync_check
participant ToolsLint as lint_tools
participant BundleClosure as lint_bundle_closure
participant CLI as node_apps_cli_dist_index
GitHubActions_ci->>Turbo: pnpm ci
activate Turbo
Turbo->>Turbo: turbo run lint typecheck test
Turbo->>DB: pnpm db:sync-check
DB-->>Turbo: exit 0 or fail on drift
Turbo->>ToolsLint: pnpm lint:tools
ToolsLint-->>Turbo: eslint tools
Turbo->>Turbo: turbo run build format:check
Turbo->>BundleClosure: pnpm lint:bundle-closure
BundleClosure-->>Turbo: node tools/bundle-closure/check.mjs
deactivate Turbo
GitHubActions_ci->>CLI: node apps/cli/dist/index.js --version
CLI-->>GitHubActions_ci: print version
GitHubActions_ci->>CLI: node apps/cli/dist/index.js --help
CLI-->>GitHubActions_ci: print help
GitHubActions_ci->>CLI: node apps/cli/dist/index.js run ... --json
CLI-->>GitHubActions_ci: run fixture with migrations resolved
Flow diagram for coverage vs coverage:enforced splitflowchart LR
A[pnpm coverage] --> B[Vitest with coverage]
A --> C[thresholds for llm, mcp, core]
D[pnpm coverage:enforced] --> E[RELAVIUM_COVERAGE_ENFORCED_ONLY=1]
E --> F[Vitest with coverage]
F --> G[thresholds for llm, mcp]
C --> H[CI and local: all three packages enforce >=90%]
G --> I[CI: llm and mcp enforce >=90%]
I --> J[core measured and printed only]
B --> H
Flow diagram for bundle-closure single-chunk assertionflowchart LR
A[Run lint:bundle-closure] --> B[node tools/bundle-closure/check.mjs]
B --> C[read metafile outputs]
C --> D[collect .js outputs]
D --> E{jsOutputs.length === 1?}
E -- No --> F[exit 1 with error about split bundle]
E -- Yes --> G[use outputKey to inspect imports]
G --> H[verify bundle closure invariants]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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:
📝 WalkthroughWalkthroughCI coverage enforcement is scoped to ChangesCI and Build Validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="package.json" line_range="20" />
<code_context>
"test": "turbo run test",
"coverage": "vitest run --coverage",
- "ci": "turbo run lint typecheck test && pnpm typecheck:tools && pnpm lint:tools && turbo run build format:check && pnpm lint:fence-check && pnpm lint:engine-deps && pnpm lint:bundle-closure",
+ "coverage:enforced": "RELAVIUM_COVERAGE_ENFORCED_ONLY=1 vitest run --coverage",
+ "ci": "turbo run lint typecheck test && pnpm db:sync-check && pnpm typecheck:tools && pnpm lint:tools && turbo run build format:check && pnpm lint:fence-check && pnpm lint:engine-deps && pnpm lint:bundle-closure",
"lint:fence-check": "node tools/lint-fixtures/assert-fence.mjs",
</code_context>
<issue_to_address>
**issue (bug_risk):** The `coverage:enforced` script uses POSIX env-var syntax, which will break on Windows shells.
On Windows shells (cmd.exe / PowerShell), this script will fail because the inline env-var assignment syntax isn’t supported. Since this command is used in CI and likely by Windows developers, consider a cross‑platform solution such as `cross-env RELAVIUM_COVERAGE_ENFORCED_ONLY=1 vitest run --coverage` or a small Node wrapper that sets the env var before running Vitest.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
6-15: 🎯 Functional Correctness | 🔵 TrivialSynchronize the coverage job’s name, comments, and branch-protection setting.
The job is now presented as
engine coverage floor (llm, mcp), while the adjacent block still says it is advisory and not required. YAML cannot change branch protection; verify that this exact status check is added as required after merge, and update the stale comment so the policy is unambiguous.Also applies to: 214-214
🤖 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 @.github/workflows/ci.yml around lines 6 - 15, Update the coverage policy comments near the coverage job to identify its exact name, “engine coverage floor (llm, mcp),” as a required status check rather than advisory, and remove any stale statement that coverage is non-required. Keep the documented llm/mcp enforcement scope and explicitly note that the matching branch-protection requirement must be configured in GitHub repository settings after merging.
🤖 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 @.github/workflows/models-catalog.yml:
- Around line 66-73: Reorder the workflow steps so “Snapshot freshness vs
upstream (informational)” runs before the mutating pnpm sync:models step. Keep
continue-on-error enabled, and preserve the existing guard behavior while
ensuring the check compares upstream against the committed snapshot before it is
rewritten.
In `@package.json`:
- Line 21: Update the root ci script to run the same compiled CLI artifact smoke
currently enforced by .github/workflows/ci.yml, extracting that smoke into a
shared script and invoking the shared script from both the package.json ci
command and the workflow path. Preserve the existing CI checks while ensuring
pnpm ci executes apps/cli/dist/index.js and catches its boot/drizzle failure.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 6-15: Update the coverage policy comments near the coverage job to
identify its exact name, “engine coverage floor (llm, mcp),” as a required
status check rather than advisory, and remove any stale statement that coverage
is non-required. Keep the documented llm/mcp enforcement scope and explicitly
note that the matching branch-protection requirement must be configured in
GitHub repository settings after merging.
🪄 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: 209bae8b-eb6c-4b1e-82bf-993f82e8920a
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/models-catalog.ymlapps/cli/tsup.config.tsapps/cli/turbo.jsondocs/standards/testing.mdpackage.jsontools/bundle-closure/check.mjsvitest.config.ts
| "coverage": "vitest run --coverage", | ||
| "ci": "turbo run lint typecheck test && pnpm typecheck:tools && pnpm lint:tools && turbo run build format:check && pnpm lint:fence-check && pnpm lint:engine-deps && pnpm lint:bundle-closure", | ||
| "coverage:enforced": "RELAVIUM_COVERAGE_ENFORCED_ONLY=1 vitest run --coverage", | ||
| "ci": "turbo run lint typecheck test && pnpm db:sync-check && pnpm typecheck:tools && pnpm lint:tools && turbo run build format:check && pnpm lint:fence-check && pnpm lint:engine-deps && pnpm lint:bundle-closure", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep pnpm ci aligned with the required artifact smoke.
The root command still never executes apps/cli/dist/index.js; a green local pnpm ci can therefore miss the compiled CLI boot/drizzle failure that .github/workflows/ci.yml now catches. Extract the smoke into a shared script and invoke it from both paths.
Suggested alignment
+ "smoke:cli": "node apps/cli/dist/index.js --version && node apps/cli/dist/index.js --help >/dev/null && node apps/cli/dist/index.js run apps/cli/src/harness/fixtures/sequential.relavium.yaml --input n=21 --json >/dev/null",
- "ci": "... && pnpm lint:bundle-closure"
+ "ci": "... && pnpm lint:bundle-closure && pnpm smoke:cli"🤖 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 `@package.json` at line 21, Update the root ci script to run the same compiled
CLI artifact smoke currently enforced by .github/workflows/ci.yml, extracting
that smoke into a shared script and invoking the shared script from both the
package.json ci command and the workflow path. Preserve the existing CI checks
while ensuring pnpm ci executes apps/cli/dist/index.js and catches its
boot/drizzle failure.
…i` is unreachable Two defects in the Wave 0 change, both found by CI doing exactly what this PR added it to do. `vitest.config.ts` read `process.env.RELAVIUM_COVERAGE_ENFORCED_ONLY` with dot notation, which `noPropertyAccessFromIndexSignature` (tsconfig.base.json) rejects — TS4111. It escaped local verification because `pnpm typecheck:tools` is a plain `tsc -p tsconfig.tools.json`, NOT part of `turbo run typecheck`, and I ran the turbo tasks individually rather than the root script this very PR exists to align. Running that script would have caught it. Attempting to run it then surfaced the second defect: `pnpm ci` does not invoke the root `ci` script at all. pnpm 9 reserves `ci` as a builtin and answers `ERR_PNPM_CI_NOT_IMPLEMENTED`, so the script is reachable only as `pnpm run ci`. Finding #312 is therefore understated: the local mirror had not merely diverged from `ci.yml`, it was unreachable by the name every contributor types, which is why the divergence could persist unnoticed. Corrected in the `ci.yml` comment and in current.md's Wave 0 entry; renaming the script is left as a maintainer call. Verified with the full `pnpm run ci` — green end to end, including the `typecheck:tools` step that failed in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…26-07-29 review A cross-document review raised 37 findings plus 5 proposal sections. Each was verified against the live files rather than taken on faith; 14 were dropped as cross-lane duplicates or declined on proportionality, and the review's own 10-step remediation order was rejected as a sequence — taken literally it puts nine documentation edits ahead of Wave 1's three CRITICALs. current.md's waves remain the execution authority. Notably, none of the findings was closed by the 2026-07-26 propagation pass: the review targets contract boundaries that pass never touched. THE FOUR BLOCKERS, all verified before acting: * 2.6.M instructed wiring the egress credential resolver into "the chat-session and workflow-run tool-environment factories" while the same document (Explicitly out of scope) and `build-engine.ts:87-89` both call the workflow-run egress/os arms permanently unwired and ADR-gated. An implementer following the plan literally would have breached a recorded security boundary. Scoped to the chat surfaces; the "every surface" acceptance clause now says "every surface that wires its capability arm". * 2.6.O routed a model-generated `.agent.yaml` through `validateAuthoredWorkflow`, which 2.6.A scopes as workflow-shaped — leaving the plan's highest-risk path (untrusted context → generated agent → tool grants → execution) with no correctly specified gate. Adds the agent-shaped `validateAuthoredAgent` with the parent-grant clamp enforced INSIDE the validator, not only at spawn, since the artifact is persisted. 2.6.O acceptance gains a reject-path criterion. * Visible session resume had no owner: deferred-tasks assigned it to "2.6.C's natural sibling or 2.6.G, whichever reaches it first", and 2.6.C shipped 2026-07-13 without it. Assigned to 2.6.G, with the repaint written into the task, the acceptance, the pull-in table and exit criterion 2. * Dynamic `invoke_workflow` was deferred by decision D57 and simultaneously carried in 2.6.P as an open "analysis gate". Recorded as deferred in the phase file and given its own deferred-tasks entry with the ADR + security review it needs to reopen. TRUTH AND AUTHORITY: * An authority notice now sits at the top of all three planning documents: current.md is canonical for progress and order, the phase files for scope and acceptance, and current.md may not redefine a security boundary or an acceptance criterion. The review found this model sound but undocumented. * Status banners under-reported: 2.5.5 still said "Planned, not started" with Wave 0 in flight; 2.6 omitted 2.6.C and 2.6.Q P1-P5. * The ADR table listed six Accepted ADRs (0067-0072) as "(new) — drafted when the workstream starts", with no link definitions. All six now resolve. * current.md described PR #76 as open, the snapshot regen as pending, and D3 as "blocked on D3b" — a row that never existed. That dangling reference was mine, introduced 2026-07-26. * Exit criterion 8 made the ABSENCE of an ADR a pass condition, which pressures against writing a genuine one. Reframed as the standing threshold: minting one is not a phase failure, skipping a genuine one is. * Exit criterion 4 named `pnpm turbo run lint typecheck test` as the gate; the real contract is `pnpm run ci` plus both required ci.yml checks. * Phase 2.5.5 claimed no new product surface while changing five public CLI contracts; they are now enumerated with the EXIT:4 reading that sanctions them. * deferred-tasks.md mixed three lifecycle states with declined items rendered as open checkboxes. Legend added, declined items converted to plain bullets. * The remaining five-language promise and the M2.6-3 milestone claim corrected. Docs-only. Two stale source comments naming the retired `MODEL_PRICING` table are folded into 2.5.5.F rather than fixed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/roadmap/phases/phase-2.5.5-hardening-and-remediation.md`:
- Line 14: Remove the standalone blank blockquote line in the authority banner
so the entire banner remains one continuous blockquote and satisfies
markdownlint MD028.
- Around line 137-139: Update the required-check statement in the phase 2.5.5
roadmap to match the current branch-protection state: either state that coverage
promotion remains pending until the “engine coverage floor (llm, mcp)” check is
required, or update the documentation only after branch protection has added it.
Keep the documented CI command and existing check names accurate.
In `@docs/roadmap/phases/phase-2.6-conversational-authoring.md`:
- Around line 913-915: Update the roadmap entry for dynamic invoke_workflow to
state that it is tracked alongside 2.6.P but deferred past Phase 2.6, rather
than moved into 2.6.P. Preserve the references to workflow composition,
subworkflow, nested-run events, and D57 while making clear it is not a 2.6.P
implementation deliverable.
🪄 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: 07f75626-24b7-4058-82f8-78ed373ed4f3
📒 Files selected for processing (4)
docs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-2.5.5-hardening-and-remediation.mddocs/roadmap/phases/phase-2.6-conversational-authoring.md
Comment on lines
+137
to
+139
| - `pnpm run ci` stays green throughout — it is `pnpm run ci`, never `pnpm ci` (pnpm reserves `ci` as a | ||
| builtin) — and so do both required `ci.yml` checks, the `ci` job including its compiled-binary smoke and | ||
| the `coverage` job (`pnpm coverage:enforced`); no item in this phase widens |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Align the documented required-check state with branch protection.
This says both CI checks are already required, while current.md still treats coverage promotion as a Wave 0 decision and the PR objective says branch protection must add engine coverage floor (llm, mcp) after merge. Clarify that these checks must pass, or update the status once branch protection is actually changed.
🤖 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 `@docs/roadmap/phases/phase-2.5.5-hardening-and-remediation.md` around lines
137 - 139, Update the required-check statement in the phase 2.5.5 roadmap to
match the current branch-protection state: either state that coverage promotion
remains pending until the “engine coverage floor (llm, mcp)” check is required,
or update the documentation only after branch protection has added it. Keep the
documented CI command and existing check names accurate.
…order the sync guards Six review findings on PR #80. Two are defects this PR introduced. * `coverage:enforced` used POSIX inline env-var syntax, which cmd.exe and PowerShell do not support — in a repo that runs a Windows CI leg. `cross-env` would be a new dependency needing an ADR (rule 2) for something ten lines of Node already do, so the script moved to `tools/coverage/enforced.mjs`, matching the existing `tools/*/check.mjs` convention. * The `sync:models:check` step I added to the weekly catalog job ran AFTER `pnpm sync:models`, which REWRITES `snapshot.ts` in the ephemeral checkout (sync.mjs's `writeFileSync`). It therefore compared upstream against the file the previous step had just regenerated and would have reported "current" every time — an informational step that could never inform. Moved ahead of the guard, where it reads the committed snapshot. * The compiled-binary smoke existed only in `ci.yml`, recreating #312's divergence in the opposite direction: the local script this PR exists to align still did not run the artifact. Extracted to `tools/cli-smoke/check.mjs` and called from both `ci.yml` and the root `ci` script, so the check cannot drift out of one of them again. * The coverage job's own comment still read "Advisory for now … promote to a required check once the margin is confirmed stable", contradicting the header this PR rewrote and the live branch protection, which now requires `engine coverage floor (llm, mcp)`. * MD028: the authority notice landed flush against the phase-2.5.5 status banner, leaving a bare blank line between two blockquotes. * 2.6.M said dynamic `invoke_workflow` was "moved to 2.6.P", which reads as a 2.6.P deliverable. It is tracked alongside 2.6.P — whose `subworkflow` node and nested-run event namespace are its natural home — but deferred past the phase (D57); 2.6.P's ADR records the deferral, not the design. SKIPPED: the finding that phase-2.5.5's "both required `ci.yml` checks" wording overclaims. Branch protection already lists both `lint · typecheck · test` and `engine coverage floor (llm, mcp)`, so the statement is accurate as written. Verified with the full `pnpm run ci` (green, including the new smoke leg) and by re-confirming the enforcement split behaviourally: with `core`'s floor raised to 99, `coverage:enforced` passes and `coverage` fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…llowed `tools/coverage/enforced.mjs` never reached the repository. `.gitignore:49` carries `coverage/` with no leading slash, so it matches a directory of that name at EVERY depth — `git add -A` skipped the new script without a word, the commit looked complete, and CI failed with MODULE_NOT_FOUND on a file that exists on every developer's disk. Renamed to `tools/coverage-gate/run.mjs`. The two obvious alternatives are both worse: anchoring the pattern to `/coverage/` would stop ignoring the per-package `packages/*/coverage` output it exists for, and a `!` negation cannot re-include a file whose parent directory is excluded. Verified the file is in the index this time, not merely on disk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@package.json`:
- Line 30: The db:sync-check script uses POSIX-only shell syntax and is not
cross-platform. Extract its migration-generation and git-status validation into
a Node-based checker such as tools/db-sync/check.mjs, preserving the existing
out-of-sync error and failure behavior, then update the package.json
db:sync-check command to invoke that checker through pnpm.
In `@tools/cli-smoke/check.mjs`:
- Around line 28-33: Update the smoke-test flow around the steps loop and
spawnSync call to resolve BUNDLE and FIXTURE to absolute paths, create a unique
temporary directory for the run, and pass it as the child process cwd so
history.db is isolated. Ensure the temporary directory is removed after all
steps complete, including when a step fails.
- Around line 32-37: Update the spawnSync invocation inside the steps loop to
include a bounded timeout option, and treat timeout termination as a failure
alongside nonzero status. When reporting the failure, include r.error details
when available before exiting, while preserving the existing label, exit status,
and stderr diagnostics.
🪄 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: 94356a0f-bf23-4073-b2c0-ba21e2e6dbe6
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/models-catalog.ymldocs/roadmap/phases/phase-2.5.5-hardening-and-remediation.mddocs/roadmap/phases/phase-2.6-conversational-authoring.mdpackage.jsontools/cli-smoke/check.mjs
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/models-catalog.yml
- docs/roadmap/phases/phase-2.5.5-hardening-and-remediation.md
- .github/workflows/ci.yml
- docs/roadmap/phases/phase-2.6-conversational-authoring.md
… Sonar sweep
Four review findings plus a Sonar batch.
* `pnpm smoke:cli` was writing to the developer's REAL `~/.relavium/history.db`.
The reviewer suggested a temporary cwd; that would not have worked — the path
is home-relative (`db/open.ts` → `paths.ts` → `os.homedir()`), not
cwd-relative, and no config overrides that root. `os.homedir()` does honour
`$HOME` / `%USERPROFILE%`, so the smoke now runs against a `mkdtemp` sandbox
and removes it in a `finally`. Verified by mtime: the real database is
untouched across a full `pnpm run ci`. This is the hazard already tracked in
deferred-tasks.md, reached from a new direction.
* The smoke also used repo-relative paths (broken from any other cwd) and had no
timeout, so a hung CLI would have sat until the CI job's own 15-minute kill
with no usable signal. Absolute paths, a 120 s per-step bound, and explicit
`error` / `signal` / `status` reporting.
* `db:sync-check` was POSIX-only shell (`test -z "$(…)" || { …; }`), the same
break-on-Windows class as `coverage:enforced`. Extracted to
`tools/db-sync/check.mjs`, keeping `git status --porcelain` — `git diff` does
not see an untracked new migration.
* `tools/coverage-gate/run.mjs` spawned a bare `vitest`, resolved through `PATH`
(Sonar), and needed `shell: true` on Windows to find the `.cmd` shim — two
injection surfaces for one convenience. It now resolves vitest's own entry
through the module graph and spawns `process.execPath` with an absolute path:
no PATH lookup, no shell.
* Deleted `ci.yml`'s reserved Phase-1 conformance lanes and their TODO. All three
adapters ship and the live nightly lane runs in `models-catalog.yml`, so the
TODO named work that had already happened elsewhere.
DECLINED — `--ignore-scripts` on the four `pnpm install` steps (8 Sonar findings).
`pnpm.onlyBuiltDependencies` in the root package.json already allowlists the only
two packages permitted to run lifecycle scripts (`better-sqlite3` for its native
prebuild, `esbuild` for its platform binary); every other transitive dependency is
already blocked. Adding the flag would break both and strengthen nothing. The
rationale is now in `ci.yml`'s header so the next sweep does not re-raise it.
ROUTED — the ~30 remaining Sonar findings are in already-merged code outside this
diff, so they go to the existing `Sonar code-quality backlog` in
deferred-tasks.md under that section's own standing policy. Two are called out as
more than cosmetic: `references.ts:154`'s regex has super-linear backtracking on
the `{{ }}` filter-argument path (2.5.5.A scope, not a nit), and
`chat-ink.tsx:1151`'s cognitive complexity of 91 belongs to the god-file 2.5.5.I
deliberately declines to decompose.
Verified with the full `pnpm run ci` — green, including the new smoke leg.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jul 29, 2026
…phase tasks done PR #80 landed the CI-truth batch; this finishes Wave 0's remaining two items and records the result where the phase documents can be read on their own. * `#162` — three skills embedded `/Users/dev/Documents/Projects/Agent-Organizer/`, a path from an unrelated project. Filed as documentation, but `add-package`'s `mkdir -p` would have created a directory tree OUTSIDE the repository, and the other two would simply fail. All three now anchor on `$(git rev-parse --show-toplevel)`, matching `write-adr` and `commit-and-pr`. * `#128`/`#129`/`#153`/`#163`/`#254` — `packages/mcp` is a shipped, tested package that appeared in none of the five inventory sites. Added to CLAUDE.md's package table, project-structure.md's table and diagram, overview.md's diagram and package list, and the reviewer agent — the last of which mattered most: its checklist never looked at the package, so an ADR-0052 boundary break had no reviewer signal. * `#164` — the reviewer agent's secrets item described only the desktop threat model (0% built) and said nothing about the surface that actually ships. Now leads with the CLI floor: stdin-not-argv, `0600` on `history.db`/`config.toml`, and redaction before anything reaches an approval preview, a run summary, or a `--json` payload. * `#167` — the security-review skill's SSRF step enumerated the ranges to block but never required REUSING the shared guard. A hand-rolled second range check is how the two drift and one silently stops covering a range (rule 3). Phase-document bookkeeping: 2.5.5.H is now 8/14 and 2.5.5.F 3/20, each task marked at its bullet so the phase file stands alone without cross-reading current.md. Wave 0 is closed; Wave 1 (the three CRITICALs and the cost-cap gaps) is next. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
Wave 0 of the Phase 2.5.5/2.6 execution order — the CI-truth batch.
One root cause across eight items: a guard that claimed a stronger guarantee than it enforced. Landed as one change because five of them edit the same file family, and splitting would leave the gate self-inconsistent in between.
cijob never ran the binary it built#294apps/cli/drizzle/**was not a declared turbo build output#315pnpm ciandci.ymlhad diverged in both directions#312#296,#152name:#320THIRD_PARTY_EXTERNALstale vspackage.json(string-width)G27,#248#314sync:models:checkimplemented, documented, called by nothing#317The two worth reading closely
#315 — the stale-artifact class.
tsup'sonSuccesscopies the migrations toapps/cli/drizzle/, outsidedist/, and turbo only restores declared outputs. A cache-hit replay could therefore leavedist/index.jsfresh beside a missingdrizzle/and crash on first DB touch, with nothing red anywhere. Verified empirically, not reasoned about: deleted the directory, replayed from cache — 14 files restored after the fix. Item #1'srun --jsonsmoke is what would now catch a regression here, which is why the two belong in one change.#296/#152 — the coverage contradiction, resolved by ruling not by wording.
llmandmcpbecome a REQUIRED check;corestays measured-and-printed but non-blocking. Its branch margin is +0.83 (90.83, measured 2026-07-29) and Phase 2.5.5's Waves 1–3 editcoreheavily — gating merges on a sub-1-point margin would red-CI real work for no defect. This is a scoped, dated ruling with a stated promotion trigger (Wave 3's test-coverage items), not an open-ended exemption. A localpnpm coveragestill enforces all three.Behaviourally verified: with
core's floor temporarily raised to 99,pnpm coverage:enforcedpasses andpnpm coveragefails — exactly the intended split.testing.mdis corrected here rather than later, because the standard asserted an enforcement the pipeline did not perform; stating the split only inci.ymlwould leave that contradiction in the standard.Maintainer action required after merge
Add
engine coverage floor (llm, mcp)to the required status checks in branch protection. The workflow change alone does not make it required — that is a repo setting, and until it is set this item is only half-landed.Conformance
Every gate run locally, exactly as CI will:
lint · typecheck · test·format:check·lint:tools·fence-check·engine-deps·bundle-closure·db:sync-check·coverage:enforced— all exit 0--version,--help, fixturerun --json) — exit 0@relavium/llmseam widening, no platform import intopackages/core, no--jsonshape changeG26(release-ref ancestry) was in Wave 0's list and already landed in #77, so it is not repeated here.🤖 Generated with Claude Code
Summary by Sourcery
Tighten CI gates so the required workflow actually validates the artifacts it ships while clarifying and partially promoting engine coverage enforcement.
New Features:
Bug Fixes:
Enhancements:
CI:
Documentation:
Summary by CodeRabbit