Conversation
…rings SkyPilot/SkyServe load task and service YAML with PyYAML, whose implicit resolvers are YAML 1.1. The yaml package's default 1.2 core schema emits values like off/no/y and sexagesimal 12:34:56 as plain scalars, which a 1.1 loader reinterprets as booleans/base-60 integers, corrupting or rejecting operator-supplied env values. Serialize with the yaml-1.1 schema, which quotes exactly those ambiguous scalars while leaving real booleans and multi-line block scalars untouched.
The origin backstop only catches host swaps; a ".." in path is resolved
by the URL parser and can climb above the base path prefix that joinUrl
exists to preserve (joinUrl("https://h/api", "/../x") -> "https://h/x")
without moving the origin. Add a second backstop asserting the resolved
pathname still sits under the prefix at a segment boundary, consistent
with the module's fail-closed posture. Every caller passes a code-literal
path today, so this is defense in depth.
detectDegradedEscalation computed degradedForMs from the tick's in-memory snapshot, but escalateDomainIfFleetIdle only guarded state='degraded' - not stateUpdatedAt. Under concurrent reconcilers, a second tick could recover then re-degrade the active (fresh stateUpdatedAt) between the first tick's snapshot load and its escalation CAS, letting the first tick escalate before the CURRENT degraded period reached degradedGraceMs. Re-check the live stateUpdatedAt against the injected clock inside the UPDATE (stateUpdatedAt < at - degradedGraceMs, strict to match core's degradedForMs > grace). Adds a cas.test.ts case modelling the race and updates the mirrored-invariant notes in failover.ts and AGENTS.md.
main() was awaited at the top level with no catch, so a missing/invalid layout file (readFileSync/JSON.parse) or a schema validation failure surfaced as an unhandled rejection. Wrap it to log a one-line message and exit(1), matching the DATABASE_URL guard's behavior.
The production DB handle is drizzle-orm/neon-http, whose HTTP driver has no interactive transactions: db.transaction() typechecks, passes on both PGlite and the real-Postgres CI lane, and throws only at runtime on Neon, which nothing in CI exercises. AGENTS.md forbids it but nothing enforced that. Add a no-restricted-syntax selector banning any .transaction( call (zero legitimate uses exist today).
The JSON Schemas and drizzle migrations are drift-gated in CI, but the prose 'haru-server environment' table was not, so a var added to or removed from serverEnvironmentSchema could silently desync from the docs AGENTS.md requires kept in sync. Parse the table's variable names and assert they equal the schema keys in both directions.
AGENTS.md forbids specific model or GPU names in code, seeds, or example layouts, but only human review enforced it. Scan packages/ and services/ sources plus example JSON for a denylist of GPU model identifiers and LLM model families. nvidia-smi (a required tool name) and vLLM (the engine) are deliberately allowed; the private-repo/infra half stays human -reviewed since the only org name present is the repo's own publisher.
The routing-commit suite only exercised the guard sequentially (commit then fail). Add a Promise.all race so the CI Postgres lane drives the genuine block-then-unblock interleave the routingCommitted-column guard exists for: the loser blocks on the operation row lock, then re-evaluates under READ COMMITTED on unblock. Asserts commit and a target_not_routed fail are never both applied, and the persisted state matches the winner, under either ordering (PGlite serializes and still proves winner/loser).
The new publishability test covers model/GPU names but leaves the private-repo/infra half of the AGENTS.md rule to human review, since the only org name in the tree is the publisher's own. Record that as a deferred item in both KNOWN_ISSUES pairs.
Follow-up to the audit-hardening tests: guard a possibly-undefined regex group, use Set#difference and non-.forEach iteration, extract deeply nested calls into locals, rename the boolean locals to the is-prefixed form unicorn requires, rename environment-docs.test.ts (docs abbreviation) to environment-contract.test.ts, and apply oxfmt. No behavior change.
Address code-review findings on the new gate: - GPU denylist now covers Blackwell (B100/B200/B300, GB300) and AMD MI300A (the MI branch accepts a trailing A or X), not just Hopper/Ampere. - LLM denylist adds falcon/nemotron/granite/internlm/baichuan/command-r/ dbrx/wizardlm/yi-N, so common open-weight families no longer slip past. - Scan all shipped data (any layout/seed/schema JSON), not only /examples/, matching the AGENTS.md 'code, seeds, or example layouts' scope. - Extract violationsInFile with a whole-file fast path (regex per line only on a real hit), flattening the former 4-level nested loop.
escalateDomainIfFleetIdle took heartbeatStaleMs and degradedGraceMs as adjacent positional numbers that default to DIFFERENT values (30s vs 60s), and every test passed them equal (30_000/30_000), so a transposed call site would compile and pass all tests while swapping the budgets in production. Collapse them into a Pick<FleetPolicy, ...> object so the fields are named (a swap is now impossible) and tied to the policy source of truth. Also extract the repeated degrade-before-grace test setup into a degradeAlphaBeforeGrace() helper.
The lint ban was bypassable (destructuring, casts, a raw drizzle handle). Omit "transaction" from HaruDatabase so any interactive-transaction call is a compile error; the full drizzle instance stays assignable (supertype). Keep the lint as a secondary guard for a call on a raw drizzle handle before it is typed, and update AGENTS.md to match.
…lace
Both renderers duplicated stringify(x, { schema: "yaml-1.1" }) and its
PyYAML rationale, coupled only by prose; one side could silently drift back
to YAML-1.2. Move the option into stringifySkyYaml in driver-skypilot
(skyserve already imports placementToResources from it) and call it from
both.
…nces The walker broke on the first line starting with '#', so a future '####' sub-note or a '#'-comment inside a fenced code block in the section would truncate the documented-var set and fail the drift gate spuriously. Break only on a same-or-higher-level heading (# / ## / ###) outside a ``` fence.
Extracting stringifySkyYaml into @haru/driver-skypilot removed skyserve's only direct `yaml` import, so the dependency is dead weight. Remove it; skyserve reaches the serializer through its existing @haru/driver-skypilot workspace dependency.
Address completeness-critic gaps from the adversarial self-review: - publishability: add a positive control (a non-trivial file set is scanned AND the full matcher flags this test file's own tokens), so a green toEqual([]) cannot mean 'scanned nothing' or 'matcher is vacuous'; add a publishability-allow line marker as an escape hatch for a legitimate token that collides with a denylist word. - environment-contract: skip the WHOLE line inside a code fence, not just the heading break, so a fenced pipe-shaped sample line is not miscounted as a documented var.
WalkthroughThe PR tightens degraded-domain escalation with an in-CAS grace check, blocks interactive transactions, adds publishability scanning, centralizes YAML 1.1 serialization, hardens URL prefix handling, improves seed errors, and adds concurrency and environment-contract tests. ChangesConcurrency and database guard updates
Publishability scanning
SkyPilot and SkyServe YAML serialization
URL path-prefix protection
Operational script and environment contract validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Code Review BotNo comment/code divergences or documentation drift detected. Reviewed 24 file(s); skipped 2. |
Greptile SummaryThis PR adds correctness fixes and automated invariant guardrails across URL handling, YAML serialization, failover escalation, database usage, seed errors, and documentation checks.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains within the scope of this follow-up review.
|
| Filename | Overview |
|---|---|
| packages/db/src/repo/domains.ts | Adds the live degraded-grace cutoff and named policy budgets to the atomic escalation guard. |
| services/haru-server/src/reconciler/reconciler.ts | Passes named escalation budgets and preserves same-tick failover behavior after a successful escalation. |
| packages/protocol/src/url.ts | Adds a segment-boundary check preventing resolved paths from escaping the base URL prefix. |
| packages/driver-skypilot/src/yaml.ts | Introduces shared YAML 1.1 serialization for Sky task and service documents. |
| packages/driver-skyserve/src/yaml.ts | Reuses the SkyPilot serializer and removes the package’s direct YAML serialization path. |
| packages/db/src/client.ts | Omits interactive transactions from the driver-agnostic database type. |
| packages/db/src/publishability.test.ts | Adds a denylist-backed repository publishability gate with parser and coverage controls. |
| services/haru-server/src/environment-contract.test.ts | Adds a drift test matching documented server environment variables to the runtime schema. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Reconcile fleet] --> B[Poll heartbeats]
B --> C{Operation in flight?}
C -- Yes --> G[Continue operation reconciliation]
C -- No --> D{Degraded escalation detected?}
D -- No --> H[Evaluate normal failover]
D -- Yes --> E[Escalation CAS checks live grace, pointer, fleet idle, and viable standby]
E --> F{CAS succeeded?}
F -- No --> H
F -- Yes --> I[Mark active failed and append audit event]
I --> H
Reviews (4): Last reviewed commit: "Close three silent-weakening paths in th..." | Re-trigger Greptile
![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
🤖 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 `@packages/db/src/publishability.test.ts`:
- Around line 55-62: Update isScannable to exclude only the scanner’s own file
by comparing each entry’s absolute path with the path derived from
fileURLToPath(import.meta.url), rather than comparing entry.name to SELF.
Preserve scanning for same-named publishability.test.ts files in other packages
or services, and remove or stop using SELF as needed.
- Around line 138-140: Replace the brittle scanned.length threshold in the
publishability coverage test with explicit assertions for representative
required files or roots, including their expected extensions. Keep the
violationsInFile(selfPath) assertion unchanged and ensure the sentinels directly
verify the scanner covers the required locations.
- Around line 46-72: The publishability gate must explicitly prove it scans the
complete governed file set instead of relying on an arbitrary count. Update
isScannable and the scan-root configuration to cover every required repository
root and text-file extension, or explicitly document and test intentional
exclusions; then replace the 20-file threshold near the gate assertion with
explicit sentinel files, roots, and extensions that verify coverage. Apply these
changes at packages/db/src/publishability.test.ts:46-72 and
packages/db/src/publishability.test.ts:138-140, using isScannable and the
existing scan assertion symbols.
- Around line 24-42: Remove the literal restricted model and GPU identifiers
from DENYLIST and its comments in the publishability test. Move these patterns
into an approved CI input/config consumed by the test, or encode the policy
without embedding the names in repository code; ensure the test still rejects
the same categories of identifiers without excluding publishability.test.ts from
scanning.
In `@packages/db/src/seed.ts`:
- Around line 65-67: Update the error formatting in the seed failure handler to
normalize embedded newline characters before passing the message to
console.error. Preserve the existing Error-versus-string extraction while
ensuring the resulting “seed failed” output is always a single line.
In `@services/haru-server/src/environment-contract.test.ts`:
- Around line 17-21: Update documentedServerVariables to accept README text as
an argument and retain only the environment-table parsing there. Move README
path construction and readFileSync into the test boundary, passing the loaded
text to documentedServerVariables so parser behavior can use fixture input
without filesystem I/O.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43a8227a-9129-421d-b63b-8a8c07cc723f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
AGENTS.mdKNOWN_ISSUES.ja.mdKNOWN_ISSUES.mdeslint.config.tspackages/core/src/failover.tspackages/db/src/cas.test.tspackages/db/src/client.tspackages/db/src/publishability.test.tspackages/db/src/repo/domains.tspackages/db/src/routing-commit.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/driver.test.tspackages/driver-skypilot/src/yaml.tspackages/driver-skyserve/package.jsonpackages/driver-skyserve/src/driver.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/protocol.test.tspackages/protocol/src/url.tsservices/haru-server/src/environment-contract.test.tsservices/haru-server/src/reconciler/reconciler.tsservices/haru-server/src/steps-race.test.ts
💤 Files with no reviewable changes (1)
- packages/driver-skyserve/package.json
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Seer Code Review
- GitHub Check: check
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep new I/O behind injectable boundaries so it can be tested without GPUs, cloud accounts, or a running database.
新しい I/O は注入可能な境界の背後に配置し、外部実行、fetch、子プロセス、タイマーなどをテストダブルに置き換えられるようにする。
**/*.{ts,tsx}: Respect the dependency graph: protocol may use only Zod and Node built-ins; core must remain pure with no I/O; db depends on core; supervisor depends on protocol only; shared server/supervisor code belongs in protocol.
Build outbound URLs withjoinUrlfrom@haru/protocol; do not usenew URL('/path', base)when the base may contain a path prefix.
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tseslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tspackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tspackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add a Vitest case next to changed code when introducing or modifying behavior.
変更したコードの近くに Vitest のテストを追加し、外部 I/O は注入可能な境界に対してテストする。
Use
@haru/db/testingwith committed migrations and avoid per-test migration calls; use injectable I/O, fake supervisors, and fake timers so tests require no GPUs, cloud accounts, or live database.
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skypilot/src/driver.test.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tspackages/db/src/routing-commit.test.tspackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tspackages/db/src/cas.test.ts
**/*.{ts,tsx,js,jsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use oxfmt as the owner of formatting, including whitespace, wrapping, quotes, and trailing commas; do not hand-tune formatting for ESLint.
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tsKNOWN_ISSUES.mdeslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tsKNOWN_ISSUES.ja.mdpackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tsAGENTS.mdpackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run both root-configured linters,
oxlint --type-awarefollowed by strict type-aware ESLint 10; add overrides at the repository root rather than per-package configs.
**/*.{ts,tsx,js,jsx}: oxlint の型認識 lint を実行した後、型情報ベースの strict ESLint 10 を実行する。設定はパッケージごとではなくリポジトリルートに置き、例外には理由をコメントしたスコープ付きオーバーライドを優先する。
コード内のコメントは英語で記述する。
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tseslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tspackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tspackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Use kebab-case for file names.
Write comments in English.
Do not use the em dash character (U+2014) in code or prose; use a colon, comma, parentheses, or spaced hyphen instead.ファイル名は kebab-case にする。
Code, comments, tests, documentation, seeds, and example layouts must not reference private consumer repositories or infrastructure, and must not contain specific model or GPU names.
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tsKNOWN_ISSUES.mdeslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tsKNOWN_ISSUES.ja.mdpackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tsAGENTS.mdpackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
services/haru-server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run server suites against in-memory PGlite with committed Drizzle migrations so compare-and-swap SQL guarding state transitions is exercised, including concurrent-winner races.
Files:
services/haru-server/src/environment-contract.test.tsservices/haru-server/src/reconciler/reconciler.tsservices/haru-server/src/steps-race.test.ts
**/*.{ts,tsx,js,jsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
oxfmt を使用して、空白、折り返し、クォート、末尾カンマを整形する。整形確認には
pnpm format:checkを使用する。
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tsKNOWN_ISSUES.mdeslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tsKNOWN_ISSUES.ja.mdpackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tsAGENTS.mdpackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
**/*.{ts,tsx,js,jsx,md}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
コードと文章ではエムダッシュ (U+2014) を使用せず、コロン、コンマ、括弧、またはスペース付きハイフンを使用する。
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tsKNOWN_ISSUES.mdeslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tsKNOWN_ISSUES.ja.mdpackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tsAGENTS.mdpackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
services/haru-server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
services/haru-server/src/**/*.{ts,tsx}: UseswitchActiveas the only writer offleets.activeDomainId, and preserve its atomic operation guard andoperations.routingCommittedupdate.
Keep the reconciler re-entrant and check-and-nudge based: issue at most one step nudge per tick, make executors safe to rerun, poll long operations, and apply outcomes through the single CAS-and-audit path.
Use the injected application clock forstepStartedAtanddomains.stateUpdatedAt; do not replace it with databasenow().
Map supervisor failures throughwithSupervisor: target-domain 401/403 responses fail immediately, other failures remain pending until policy expiry, source-step failures remain pending, and response parsing must stay inside SupervisorError handling.
Degraded escalation must retain all guards inside the escalation update: grace period, auto-failover, viable standby, no in-flight operation, and an unchanged active routing pointer.
Completion checks over supervisor-reported lists must requirelength > 0 && every(...); empty lists must not produce vacuous success.
Keep vLLM sleep/wake endpoints private and localhost-only; expose external control through the bearer-authenticated supervisor API, and have the chat proxy construct only/v1/chat/completionspaths.
The chat proxy must forward request bodies as raw text, copy onlycontent-typeplus the stale-routing header, bound only TTFB with the abort timer, abort upstream on pre-header disconnect, and use lowercasemodelrouting keys withfindRoutableBinding.
The data path may fail open only on an unreachable state store using the last valid cached snapshot; control routes must fail closed. A successful pointer read followed by a moved revision or malformed snapshot must never serve stale data.
/healthzmust never access the database, and pointer lookup exceptions must remain distinct from anullresult:nullmeans the fleet is gone, while an exception means retain the cache entry.
Use case-i...
Files:
services/haru-server/src/environment-contract.test.tsservices/haru-server/src/reconciler/reconciler.tsservices/haru-server/src/steps-race.test.ts
**/*.{ts,tsx,md}
📄 CodeRabbit inference engine (AGENTS.md)
Use English code comments and avoid the em dash character U+2014 in code and prose.
Files:
services/haru-server/src/environment-contract.test.tspackages/driver-skyserve/src/yaml.tspackages/protocol/src/url.tspackages/driver-skypilot/src/driver.test.tspackages/db/src/seed.tspackages/driver-skypilot/src/yaml.tsKNOWN_ISSUES.mdeslint.config.tspackages/db/src/client.tspackages/driver-skyserve/src/driver.test.tspackages/protocol/src/protocol.test.tsservices/haru-server/src/reconciler/reconciler.tspackages/db/src/routing-commit.test.tspackages/core/src/failover.tsKNOWN_ISSUES.ja.mdpackages/db/src/publishability.test.tsservices/haru-server/src/steps-race.test.tsAGENTS.mdpackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
packages/driver-*/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test driver integrations through an injectable
execboundary, covering recorded argv, timeout propagation, and error mapping; do not require askybinary.
Files:
packages/driver-skyserve/src/yaml.tspackages/driver-skypilot/src/driver.test.tspackages/driver-skypilot/src/yaml.tspackages/driver-skyserve/src/driver.test.ts
packages/driver-*/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Drivers must not call cloud APIs; they must render placement data to SkyPilot/SkyServe YAML and execute through injectable argv-array
execwithout a shell.
Files:
packages/driver-skyserve/src/yaml.tspackages/driver-skypilot/src/driver.test.tspackages/driver-skypilot/src/yaml.tspackages/driver-skyserve/src/driver.test.ts
packages/db/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test
@haru/dbagainst in-memory PGlite using the committed Drizzle migrations, including compare-and-swap SQL and concurrent-winner races.状態ストアの状態遷移では、compare-and-swap SQL によって並行実行時の勝者決定レースを保護する。
Schema edits must be accompanied by generated committed migrations under
packages/db/drizzle; migration drift must not remain.
Files:
packages/db/src/seed.tspackages/db/src/client.tspackages/db/src/routing-commit.test.tspackages/db/src/publishability.test.tspackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
packages/db/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/db/src/**/*.{ts,tsx}: Implement every database state transition as a single-statement compare-and-swap, check the affected row count, and never use interactivedb.transaction()or hold external work between a read and dependent write.
Keep one in-flight operation per fleet, preservesourceDomainIdat creation, and use it for post-commit cleanup rather than inferring the other domain.
Files:
packages/db/src/seed.tspackages/db/src/client.tspackages/db/src/routing-commit.test.tspackages/db/src/publishability.test.tspackages/db/src/repo/domains.tspackages/db/src/cas.test.ts
**/{README,CONTRIBUTING,KNOWN_ISSUES}.md
📄 CodeRabbit inference engine (AGENTS.md)
Maintain English/Japanese documentation pairs together; update the corresponding
.ja.mdfile whenever the English document changes.
Files:
KNOWN_ISSUES.md
packages/core/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
For state-machine changes in
@haru/core, extend the exhaustive transition-table tests.状態機械を変更する場合は、すべての状態遷移を網羅する遷移テーブルテストを拡張する。
Files:
packages/core/src/failover.ts
packages/core/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Treat the state tables in
slot-state.tsanddomain-state.tsas the single source of truth; repository code must reject invalid transitions, and shared predecessor lists must derive fromstatesWithEdgeTo.
Files:
packages/core/src/failover.ts
🪛 LanguageTool
KNOWN_ISSUES.md
[typographical] ~215-~215: The word ‘Where’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ...packages/db/src/publishability.test.ts. - Current: the gate scans sources and e...
(WRB_QUESTION_MARK)
[typographical] ~223-~223: The word ‘Why’ starts a question. Add a question mark (“?”) at the end of the sentence.
Context: ... safely enumerated here without guessing. - Intended fix: when the consumer-org i...
(WRB_QUESTION_MARK)
AGENTS.md
[typographical] ~127-~127: To join two clauses or introduce examples, consider using an em dash.
Context: ...ride inside the escalation UPDATE itself - the grace re-checks the live `stateUpd...
(DASH_RULE)
🪛 OpenGrep (1.25.0)
services/haru-server/src/environment-contract.test.ts
[ERROR] 51-51: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
packages/db/src/publishability.test.ts
[ERROR] 109-109: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (19)
packages/driver-skypilot/src/yaml.ts (1)
20-35: LGTM!Also applies to: 48-48
packages/driver-skyserve/src/yaml.ts (1)
1-10: LGTM!Also applies to: 26-26
packages/driver-skypilot/src/driver.test.ts (1)
78-99: LGTM!packages/driver-skyserve/src/driver.test.ts (1)
61-74: LGTM!AGENTS.md (1)
74-81: LGTM!Also applies to: 126-130
eslint.config.ts (1)
123-138: LGTM!packages/db/src/client.ts (1)
15-26: LGTM!packages/db/src/repo/domains.ts (1)
10-10: LGTM!Also applies to: 19-19, 57-59, 76-99, 114-114, 150-150
packages/core/src/failover.ts (1)
95-100: LGTM!services/haru-server/src/reconciler/reconciler.ts (1)
644-659: LGTM!packages/db/src/cas.test.ts (1)
33-45: LGTM!Also applies to: 198-212, 223-243, 248-268, 273-318
services/haru-server/src/steps-race.test.ts (1)
350-358: LGTM!Also applies to: 374-389
packages/db/src/routing-commit.test.ts (1)
185-240: LGTM!services/haru-server/src/environment-contract.test.ts (1)
64-65: 🎯 Functional CorrectnessNo change needed. The repository targets Node
>=24.10.0, andSet.prototype.differenceis available in that baseline.packages/db/src/publishability.test.ts (1)
1-23: LGTM!Also applies to: 75-126, 128-137, 139-142
KNOWN_ISSUES.md (1)
213-227: LGTM!KNOWN_ISSUES.ja.md (1)
202-216: LGTM!packages/protocol/src/url.ts (1)
42-58: LGTM!packages/protocol/src/protocol.test.ts (1)
686-708: LGTM!
| // The scanner file names the very tokens it bans; excluding it keeps the | ||
| // gate from flagging itself. | ||
| const SELF = "publishability.test.ts"; | ||
|
|
||
| function isScannable(name: string): boolean { | ||
| if (name === SELF) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Exclude only the exact scanner file.
SELF is compared with entry.name, so any other publishability.test.ts under packages or services is silently skipped. A denylisted token in that file would pass the gate. Compare the absolute path with fileURLToPath(import.meta.url) instead.
🤖 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 `@packages/db/src/publishability.test.ts` around lines 55 - 62, Update
isScannable to exclude only the scanner’s own file by comparing each entry’s
absolute path with the path derived from fileURLToPath(import.meta.url), rather
than comparing entry.name to SELF. Preserve scanning for same-named
publishability.test.ts files in other packages or services, and remove or stop
using SELF as needed.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bbf5fa287
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Instinct (MI250/MI300A/MI300X/MI325X), and RTX consumer cards. The | ||
| // MI branch allows a trailing A (APU) or X (accelerator) suffix. | ||
| pattern: | ||
| /\b(?:H100|H200|H800|B100|B200|B300|A100|A800|A6000|A40|V100|L40S|L40|GH200|GB200|GB300|MI\d{2,3}[AX]?|RTX ?\d{3,4})\b/i, |
There was a problem hiding this comment.
This new publishability gate hard-codes the exact GPU identifiers it is meant to keep out of the repository, and the model-family regex below does the same for LLM names. Since this file is test code and the scanner deliberately excludes itself, CI will stay green while the repository now contains the specific workload names the project forbids; move the denylist out of the publishable tree or encode the detector without literal model/GPU names.
AGENTS.md reference: AGENTS.md:L212-L215
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 22 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Address PR review (CodeRabbit/Codex/cubic) on the publishability gate: - The gate embedded the exact GPU/LLM identifiers it forbids in test CODE and excluded itself from scanning. Move the patterns into publishability-denylist.txt, a policy DATA file that is not a scanned source (AGENTS.md scopes the rule to code/seeds/layouts), so no model or GPU name lives in governed code and the self-exclusion hack is gone. - Cover every shipped module extension (.ts/.mts/.cts/.mjs/.cjs/.js), not only .ts, so a leak in e.g. generate-schemas.mjs no longer passes. - Replace the brittle >20 file-count positive control with explicit coverage sentinels (representative files across both roots and every kind) plus a non-vacuous check that each pattern matches its own policy.
A multi-issue Zod error or a JSON.parse error carries embedded newlines, so the 'clean one-line message' the guard promises could still span lines. Collapse CR/LF runs to a single space.
Per the injectable-I/O testing convention, documentedServerVariables now takes README TEXT (the file read stays at the test boundary), so the section walker can be exercised with inline fixtures. Add cases for a sub-note heading, a fenced pipe-shaped sample line, and a missing section - the edges the walker hardening was written for.
…trol Adversarial self-review found the data-file move traded one vacuous-pass risk for three worse ones, all silent: - A policy file that parsed to zero rules (comments-only after a bad merge or truncation) left DENYLIST empty; `[].every(...)` is vacuously true, so every file scanned clean and BOTH tests stayed green with a real leak in the tree. - A row written with spaces instead of a TAB compiled the WHOLE line as the regex - a dead rule that still matched its own policy text, so the self-check passed while it matched nothing in the repo. A CRLF checkout did the same via a trailing carriage return. - The rewritten positive control asserted the raw regexes against the policy text and no longer called `violationsInFile`, leaving the actual scan pipeline untested: stubbing it to return [] kept the suite green. Parse through a pure `parseDenylist(text)` that strips \r and THROWS on a missing TAB, an empty pattern, or zero rules; assert the end-to-end pipeline by scanning the policy file itself (it carries the tokens) and requiring a reported hit per rule; add a test for each rejection. All four mutants (dead scanner, inverted fast path, emptied policy, space-separated row) now fail the suite.
The completeness critic showed the per-RULE non-vacuity check was still too weak: both rules are large alternations, so deleting a single branch (say the qwen alternative) left 'the pattern matches its own policy text' true while that identifier stopped being detected. Add publishability-samples.txt - one identifier per branch, governed data like the policy itself - and assert every sample is reported through the real matcher, which now lives in a pure violationsInText. Also close the half-satisfied review ask: scan .github and the root-level files (README/AGENTS/KNOWN_ISSUES/workflows) and the .md/.yaml/.yml extensions, not just packages/services .ts, and pin isScannable's verdicts - including the deliberate exclusions - in a test rather than leaving four of six extensions aspirational. AGENTS.md now documents the sanctioned data-file exemption that makes the design legal.
The newline collapse shipped with no coverage at all (seed.ts is a script with a top-level await, so importing it to test would run the seed). Move the formatting into seed-error.ts and test it against a real multi-issue Zod error, which carries indented continuation lines - so collapse every whitespace run, not just line breaks, or the one-line message comes out with wide gaps mid-sentence.
An odd number of fences left isInFence true to EOF, swallowing the rest of the section and the heading that ends it; the drift test then failed with a baffling 'undocumented env var' diff instead of the markdown typo it is. Throw on the unbalanced fence, and cover the two walker cases the fixtures missed: a section ended by a higher-level heading, and the unterminated fence itself.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@AGENTS.md`:
- Around line 223-224: The documentation and scanner behavior around the
publishability exemption must bind publishability-allow to the exact legitimate
collidee. Update the relevant denylist scanning logic and AGENTS.md guidance so
the marker is honored only when it appears on the same line as a governed
denylist token/rule collision, and add tests proving unrelated prohibited
references cannot bypass the gate.
In `@packages/db/src/publishability.test.ts`:
- Around line 58-62: Update the policy parsing logic around the denylist rule
construction to reject labels that are empty or contain only whitespace, before
adding the rule to rules. Preserve the existing empty-pattern validation and add
a malformed-policy test covering a blank label such as a tab-prefixed pattern.
- Around line 160-165: Add a Vitest regression case next to the publishability
matcher tests that compares equivalent marked and unmarked lines: assert the
line containing the `publishability-allow` marker produces no finding, while the
unmarked version is reported. Use the existing matcher/test helpers and preserve
their established assertion style.
In `@packages/db/src/seed-error.test.ts`:
- Around line 14-16: In the seed-error test, narrow the safeParse result after
the parsed.success assertion before accessing parsed.error in the
formatSeedError call. Add an explicit control-flow check or assertion that
establishes the failure branch, while preserving the existing runtime
expectation and error formatting behavior.
In `@packages/db/src/seed.ts`:
- Around line 66-67: Update main() error handling so the database client is
closed before any process.exit(1) call, including errors occurring after client
creation. Ensure cleanup runs reliably before exiting while preserving the
existing formatted error logging.
In `@services/haru-server/src/environment-contract.test.ts`:
- Line 41: Update the empty-section validation in the relevant
environment-contract test fixtures so it checks documented.size === 0 regardless
of isEnded, preserving the expected error when a heading terminates an otherwise
empty section. Add an empty-section fixture covering termination by ###, ##, or
# and apply the same correction to the other referenced cases.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ff01af66-414e-49b7-b9c4-38fb5721c974
📒 Files selected for processing (8)
AGENTS.mdpackages/db/src/publishability-denylist.txtpackages/db/src/publishability-samples.txtpackages/db/src/publishability.test.tspackages/db/src/seed-error.test.tspackages/db/src/seed-error.tspackages/db/src/seed.tsservices/haru-server/src/environment-contract.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Seer Code Review
- GitHub Check: check
- GitHub Check: test-postgres
🧰 Additional context used
📓 Path-based instructions (14)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Use kebab-case for file names.
Write comments in English.
Do not use the em dash character (U+2014) in code or prose; use a colon, comma, parentheses, or spaced hyphen instead.ファイル名は kebab-case にする。
Keep the repository publishable: do not reference consumer-private repositories or infrastructure, and do not include specific model or GPU names in code, seeds, layouts, tests, comments, or documentation except in the governed publishability denylist and samples files.
Files:
packages/db/src/publishability-denylist.txtpackages/db/src/publishability-samples.txtpackages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsAGENTS.mdservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep new I/O behind injectable boundaries so it can be tested without GPUs, cloud accounts, or a running database.
新しい I/O は注入可能な境界の背後に配置し、外部実行、fetch、子プロセス、タイマーなどをテストダブルに置き換えられるようにする。
**/*.{ts,tsx}: Respect the dependency graph:@haru/protocolcontains shared protocol helpers and types;@haru/coreis pure logic with no I/O;@haru/dbdepends on core; services may use the appropriate lower layers;@haru-supervisordepends only on protocol.
Build outbound URLs withjoinUrlfrom@haru/protocol; do not usenew URL('/path', base)when the base may contain a path prefix.
Use the single rooteslint.config.tsandoxlint.config.ts; add scoped overrides at the root with a reason instead of package-local configs.
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
**/*.{ts,tsx,js,jsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use oxfmt as the owner of formatting, including whitespace, wrapping, quotes, and trailing commas; do not hand-tune formatting for ESLint.
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsAGENTS.mdservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run both root-configured linters,
oxlint --type-awarefollowed by strict type-aware ESLint 10; add overrides at the repository root rather than per-package configs.
**/*.{ts,tsx,js,jsx}: oxlint の型認識 lint を実行した後、型情報ベースの strict ESLint 10 を実行する。設定はパッケージごとではなくリポジトリルートに置き、例外には理由をコメントしたスコープ付きオーバーライドを優先する。
コード内のコメントは英語で記述する。
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
packages/db/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Test
@haru/dbagainst in-memory PGlite using the committed Drizzle migrations, including compare-and-swap SQL and concurrent-winner races.状態ストアの状態遷移では、compare-and-swap SQL によって並行実行時の勝者決定レースを保護する。
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tspackages/db/src/publishability.test.ts
**/*.{ts,tsx,js,jsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
oxfmt を使用して、空白、折り返し、クォート、末尾カンマを整形する。整形確認には
pnpm format:checkを使用する。
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsAGENTS.mdservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
**/*.{ts,tsx,js,jsx,md}
📄 CodeRabbit inference engine (CONTRIBUTING.ja.md)
コードと文章ではエムダッシュ (U+2014) を使用せず、コロン、コンマ、括弧、またはスペース付きハイフンを使用する。
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsAGENTS.mdservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
packages/db/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
packages/db/src/**/*.ts: Implement every state transition as a single-statement compare-and-swap with an appropriateWHEREpredicate and verify the affected row count. Do not use interactivedb.transaction()throughHaruDatabaseor hold external work between a read and dependent write.
Preserve the one-in-flight-operation-per-fleet invariant, join the existing in-flight row on conflict, and recordsourceDomainIdat operation creation for post-commit cleanup.
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tspackages/db/src/publishability.test.ts
**/*.{ts,tsx,md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
Use English code comments and avoid the em dash character U+2014 in code and prose.
Files:
packages/db/src/seed-error.tspackages/db/src/seed.tspackages/db/src/seed-error.test.tsAGENTS.mdservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Add a Vitest case next to changed code when introducing or modifying behavior.
変更したコードの近くに Vitest のテストを追加し、外部 I/O は注入可能な境界に対してテストする。
Files:
packages/db/src/seed-error.test.tsservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.test.{ts,tsx}: Use@haru/db/testingand its committed migrations for database tests; do not add per-test migration calls. State-machine changes must extend exhaustive transition-table tests.
Keep tests independent of GPUs, cloud accounts, and live databases by injecting fetch, exec, spawn, and clock dependencies; use Honoapp.request()and fake supervisors for server tests.
Files:
packages/db/src/seed-error.test.tsservices/haru-server/src/environment-contract.test.tspackages/db/src/publishability.test.ts
services/haru-server/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Run server suites against in-memory PGlite with committed Drizzle migrations so compare-and-swap SQL guarding state transitions is exercised, including concurrent-winner races.
Files:
services/haru-server/src/environment-contract.test.ts
services/haru-server/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
services/haru-server/src/**/*.ts: UseswitchActiveas the only writer offleets.activeDomainId, and preserve its atomic routing-commit and operation-guard behavior.
Use the injected application clock forstepStartedAtanddomains.stateUpdatedAt; do not replace it with databasenow().
Route supervisor failures throughwithSupervisor: target-domain 401/403 responses fail the step immediately, while other failures remain pending until the policy budget expires; source steps treat authentication failures as pending. Keep response parsing inside SupervisorError wrapping.
Mirror supervisor-reported per-slot health using guarded CAS transitions; a running training report without a PID must not count as healthy.
Keep vLLM sleep/wake admin endpoints private and loopback-only; expose external control only through the supervisor's bearer-authenticated API.
Usemodelas a lowercase routing key and resolve it through the same per-model predicate route intent uses, includingfindRoutableBindingin core.
The data path may fail open only when the pointer read fails: serve the last valid cached snapshot withX-Haru-Routing: stale; control routes must fail closed.
After a successful pointer read, fail closed if the pointer revision moved or the snapshot is malformed; only a revision-matching snapshot cached at the current load may be served.
Distinguish a pointer lookup returningnullfrom throwing:nullmeans the fleet is gone and may be forgotten; a thrown lookup means the store is unavailable and the cache entry must be retained.
Fleet-reference identity must be case-insensitive for UUID IDs but case-sensitive for slugs and aliases; cache indexing must preserve the database ID-first rule usingisFleetIdShaped.
Do not publish an overlapping snapshot load when the fleet was forgotten, a newer revision was cached, or a slug-fallback result has a different ID; retain later knowledge while building only the response from the losing snapshot.
Files:
services/haru-server/src/environment-contract.test.ts
services/haru-server/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
services/haru-server/src/**/*.{ts,tsx}: The chat proxy must forward request bodies as raw text, copy onlycontent-typeplus the permitted stale-routing header, bound only TTFB with its abort timer, abort upstream on pre-header client disconnect, and construct only/v1/chat/completionspaths.
/healthzmust never access the database.
Files:
services/haru-server/src/environment-contract.test.ts
🪛 ast-grep (0.44.1)
packages/db/src/publishability.test.ts
[warning] 61-61: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(source, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 61-61: Do not use variable for regular expressions
Context: new RegExp(source, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity. Security best practice.
(regexp-non-literal-typescript)
🪛 LanguageTool
packages/db/src/publishability-denylist.txt
[style] ~4-~4: Since ownership is already implied, this phrasing may be redundant.
Context: ... so the publishability gate never flags its own policy. # (AGENTS.md scopes the no-mode...
(PRP_OWN)
[grammar] ~7-~7: Please add a punctuation mark at the end of paragraph.
Context: ...x\b|\bwizardlm\b|\byi-\d|\b(?:gpt|phi)-\d
(PUNCTUATION_PARAGRAPH_END)
packages/db/src/publishability-samples.txt
[style] ~4-~4: Since ownership is already implied, this phrasing may be redundant.
Context: ...ives are deleted, so every branch needs its own # sample here. Like the policy file thi...
(PRP_OWN)
AGENTS.md
[uncategorized] ~217-~217: The official name of this software platform is spelled with a capital “H”.
Context: ...u/db, scanning the packages/services/.github trees plus the root-level files). Its O...
(GITHUB)
🔇 Additional comments (7)
packages/db/src/publishability-denylist.txt (1)
1-7: LGTM!packages/db/src/publishability-samples.txt (1)
1-47: LGTM!packages/db/src/publishability.test.ts (1)
10-57: LGTM!Also applies to: 70-159, 177-309
packages/db/src/seed-error.ts (1)
1-15: LGTM!packages/db/src/seed-error.test.ts (1)
1-13: LGTM!Also applies to: 17-32
packages/db/src/seed.ts (1)
6-6: LGTM!services/haru-server/src/environment-contract.test.ts (1)
16-18: LGTM!Also applies to: 20-25, 27-28, 64-71, 82-84, 100-154
| sample whenever you add a denylist branch. A line with a legitimate | ||
| colliding token opts out with a `publishability-allow` marker. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'publishability-allow|publishability-(denylist|samples)' \
packages/db/srcRepository: arkorlab/haru
Length of output: 6762
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- packages/db/src/publishability.test.ts lines 120-180 ---'
sed -n '120,180p' packages/db/src/publishability.test.ts | nl -ba -v120
printf '%s\n' '--- tests around publishability-allow ---'
rg -n -C 6 'publishability-allow|allow' packages/db/src/publishability.test.ts
printf '%s\n' '--- sample/denylist files ---'
for f in packages/db/src/publishability-samples.txt packages/db/src/publishability-denylist.txt; do
if [ -f "$f" ]; then
printf '\n### %s\n' "$f"
wc -l "$f"
sed -n '1,220p' "$f"
fi
done
printf '%s\n' '--- AGENTS section lines 210-230 ---'
sed -n '210,230p' AGENTS.md | nl -ba -v210Repository: arkorlab/haru
Length of output: 252
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
tests = Path("packages/db/src/publishability.test.ts").read_text()
print("contains allow before scanning hit:", "if (line.includes(\"publishability-allow\"))" in tests)
print("contains same-line collision check:", any(x in tests for x in [
'publishability-allow',
'line === "publishability-allow"',
'trim() === "publishability-allow"',
'includes("publishability-allow") && .*pattern',
]))
# Extract loop body roughly and print whether allow branch has an enclosing denylist-pattern test before continue.
start = tests.index('for (const [index, line] of content.split("\\n").entries())')
block = tests[start:start + 120_000]
print("block contains allow continuation:", 'if (line.includes("publishability-allow"))' in block)
print("block contains scan:", 'for (const { label, pattern } of DENYLIST)' in block)
allow_before_pattern = block.index('if (line.includes("publishability-allow"))') < block.index('for (const { label, pattern } of DENYLIST)')
print("allow exception occurs before denylist patterns are checked:", allow_before_pattern)
PYRepository: arkorlab/haru
Length of output: 365
Tie publishability-allow to the exact collidee.
The scanner currently skips any line containing publishability-allow before checking the denylist. Document and test that the exemption only applies on the same line to a legitimate collision with a governed denylist token/rule, so unrelated prohibited references cannot bypass the gate.
🤖 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 `@AGENTS.md` around lines 223 - 224, The documentation and scanner behavior
around the publishability exemption must bind publishability-allow to the exact
legitimate collidee. Update the relevant denylist scanning logic and AGENTS.md
guidance so the marker is honored only when it appears on the same line as a
governed denylist token/rule collision, and add tests proving unrelated
prohibited references cannot bypass the gate.
Source: Coding guidelines
| for (const [index, line] of content.split("\n").entries()) { | ||
| // Escape hatch: a line with a legitimate token that collides with a | ||
| // denylist word opts out with a `publishability-allow` marker. None | ||
| // exist today. | ||
| if (line.includes("publishability-allow")) { | ||
| continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a direct allow-marker test.
The documented escape hatch skips a matching line, but no Vitest case verifies that marked content is suppressed while the equivalent unmarked content is reported. Add this regression case next to the matcher tests. As per coding guidelines: “Add a Vitest case next to changed code when introducing or modifying behavior.”
🤖 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 `@packages/db/src/publishability.test.ts` around lines 160 - 165, Add a Vitest
regression case next to the publishability matcher tests that compares
equivalent marked and unmarked lines: assert the line containing the
`publishability-allow` marker produces no finding, while the unmarked version is
reported. Use the existing matcher/test helpers and preserve their established
assertion style.
Source: Coding guidelines
| expect(parsed.success).toBe(false); | ||
|
|
||
| const formatted = formatSeedError(parsed.error); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching seed-error.test.ts =="
fd -a 'seed-error\.test\.ts$' . || true
echo "== package/config files for TypeScript type-checking =="
git ls-files | rg '(^|/)(tsconfig.*\.json|tsconfig.*\.js|eslint\.config\.(ts|js|mjs|cjs)|oxlint\.config\.(ts|js)|package\.json)$' | head -200
echo "== inspect test file context =="
for f in $(fd 'seed-error\.test\.ts$' .); do
echo "--- $f"
wc -l "$f"
cat -n "$f"
done
echo "== inspect seed-error implementation and formatSeedError =="
fd 'seed-error' . | while read -r f; do
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
doneRepository: arkorlab/haru
Length of output: 4800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant tsconfig/build files =="
for f in tsconfig.base.json packages/db/package.json packages/db/tsconfig.json packages/db/tsconfig.build.json tsconfig.base.json; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,220p' "$f"
fi
done
echo "== scripts mentioning typecheck/compile/tsc =="
python3 - <<'PY'
import json, pathlib
for p in [pathlib.Path('.package.json')] + list(pathlib.Path('.').rglob('package.json')):
try:
data=json.loads(p.read_text())
except Exception:
continue
scripts=data.get('scripts',{})
if any(k in scripts for k in ['typecheck','compile','build','test']):
print(f'--- {p}')
for k,v in data.get('scripts',{}).items():
if any(s in k.lower() or any(t in s.lower() for t in ['tsc','typecheck','compile','test','vitest']) for s in [k,v]):
print(k,':',v)
PY
echo "== package deps typescript/zod/vitest availability =="
node - <<'JS'
const fs = require('fs');
const path = require.resolve('typescript', { paths: ['./packages/db','node_modules'] });
console.log('typescript-path:', path);
console.log('typescript-version:', require('typescript').version);
try {
console.log('zod-path:', require.resolve('zod', { paths: ['./packages/db','node_modules'] }));
} catch(e) {
console.log('zod-path:', '<not available>');
}
try {
console.log('vitest-path:', require.resolve('vitest', { paths: ['./packages/db','node_modules'] }));
} catch(e) {
console.log('vitest-path:', '<not available>');
}
JSRepository: arkorlab/haru
Length of output: 5751
🌐 Web query:
Zod safeParse success false type narrowing parsed.error success discriminator
💡 Result:
The Zod safeParse method returns a discriminated union, which allows for robust type narrowing [1][2]. The object returned by safeParse contains a success property that serves as the discriminant [3][4]. To correctly narrow the type and access the error or data properties, you must check the value of success [3][1]. TypeScript's control flow analysis will then automatically narrow the object type, enabling safe access to the relevant properties [5]. Example of proper handling: const result = schema.safeParse(input); if (!result.success) { // TypeScript narrows result to ZodSafeParseError // You can safely access result.error here [3][1] console.error(result.error); } else { // TypeScript narrows result to ZodSafeParseSuccess // You can safely access result.data here [1][2] console.log(result.data); } Important Requirements: 1. TypeScript Configuration: This type narrowing behavior relies on TypeScript's strict mode (specifically strictNullChecks set to true) [3][5]. If you encounter issues where error is not accessible even after a !result.success check, ensure your project's tsconfig.json has strict mode enabled [3][5]. 2. Discriminated Union Design: Because safeParse returns a discriminated union, accessing result.error directly without first checking result.success (or !result.success) will result in a TypeScript error, as error does not exist on the success variant of the union [3][4]. Similarly, data does not exist on the error variant [6][4]. If you are using array methods like .filter(), the type narrowing might not persist automatically [7]. In those cases, you may need to use a type predicate to inform TypeScript about the narrowed type [7].
Citations:
- 1: https://github.com/colinhacks/zod/blob/v4.4.3/packages/docs/content/basics.mdx
- 2: https://zod.dev/basics
- 3: safeParse discriminated union doesn't have 'error' attribute colinhacks/zod#1190
- 4: Different typing for SafeParseReturnType<T> colinhacks/zod#3266
- 5: Can't access error property on safeParse() result? colinhacks/zod#1080
- 6: https://github.com/colinhacks/zod/blob/main/packages/zod/src/v4/classic/parse.ts
- 7: Type Narrowing Not Retained in Chained Array Methods with safeParse colinhacks/zod#3145
🌐 Web query:
Vitest expect boolean type narrowing TypeScript
💡 Result:
Standard Vitest expect matchers, such as .toBeTruthy() or .toBeFalsy(), do not perform TypeScript type narrowing at compile time [1][2]. Because expect is designed for runtime assertions, it does not inherently communicate type information to the TypeScript compiler. To achieve type narrowing within your tests, use the following approaches: 1. Use expect.assert(booleanExpression) For runtime checks that also inform TypeScript's control flow analysis (type narrowing), use expect.assert(). It accepts any boolean expression and applies the same type narrowing that TypeScript's compiler would apply within an if branch [1][3]. This is the recommended method for narrowing types during test execution [4]. ts const value: string | null = getValue(); expect.assert(value !== null); // TypeScript now knows 'value' is string console.log(value.toUpperCase()); 2. Use expectTypeOf for Compile-Time Checks If your goal is to test that a type is narrowed correctly (rather than narrowing it for runtime test logic), use expectTypeOf inside a dedicated type test file (e.g., .test-d.ts) and run Vitest with the --typecheck flag [5][6][7]. ts import { expectTypeOf } from 'vitest'; // Assert that the type is correctly narrowed expectTypeOf(value).toEqualTypeOf<string>(); Note: Do not rely on runtime expect matchers to influence TypeScript types. If you require strict boolean checking to avoid type coercion issues in tests, prefer using .toBe(true) or .toBe(false) over .toBeTruthy() and .toBeFalsy() [8].
Citations:
- 1: https://vitest.dev/api/expect.html
- 2: expect(x).toBeTruthy() should properly narrow typescript types to be non-falsy vitest-dev/vitest#2883
- 3: https://main.vitest.dev/guide/recipes/type-narrowing
- 4: feat: support
expect.assertfor type narrowing vitest-dev/vitest#8695 - 5: https://vitest.dev/guide/testing-types
- 6: https://vitest.dev/api/expect-typeof
- 7: https://qaskills.sh/blog/vitest-expect-typeof-typescript-type-tests
- 8: https://github.com/vitest-dev/eslint-plugin-vitest/blob/main/docs/rules/prefer-strict-boolean-matchers.md
Narrow the safeParse result before accessing error.
expect(parsed.success).toBe(false) is a runtime assertion and does not narrow TypeScript’s safeParse discriminated union, so parsed.error on Line 16 is still a type error.
Proposed fix
expect(parsed.success).toBe(false);
+ if (parsed.success) {
+ throw new Error("expected invalid fixture");
+ }
const formatted = formatSeedError(parsed.error);📝 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.
| expect(parsed.success).toBe(false); | |
| const formatted = formatSeedError(parsed.error); | |
| expect(parsed.success).toBe(false); | |
| if (parsed.success) { | |
| throw new Error("expected invalid fixture"); | |
| } | |
| const formatted = formatSeedError(parsed.error); |
🤖 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 `@packages/db/src/seed-error.test.ts` around lines 14 - 16, In the seed-error
test, narrow the safeParse result after the parsed.success assertion before
accessing parsed.error in the formatSeedError call. Add an explicit control-flow
check or assertion that establishes the failure branch, while preserving the
existing runtime expectation and error formatting behavior.
| console.error(formatSeedError(error)); | ||
| process.exit(1); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate seed.ts =="
fd -a 'seed\.ts$' . || true
echo "== file outline =="
ast-grep outline packages/db/src/seed.ts --view expanded || true
echo "== relevant seed.ts lines =="
cat -n packages/db/src/seed.ts | sed -n '1,140p'
echo "== process.exitCode/process.exit usage in db =="
rg -n "process\.(exit|exitCode)|db\.close|disconnect|client\.close" packages/db/src packages/db/test 2>/dev/null | head -200 || true
echo "== package files for node/db drivers =="
for f in package.json packages/db/package.json; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,220p' "$f"
fi
doneRepository: arkorlab/haru
Length of output: 6407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read remaining seed.ts if any =="
wc -l packages/db/src/seed.ts
cat -n packages/db/src/seed.ts | sed -n '140,260p'
echo "== imports and db symbol usages =="
rg -n "from ['\"].*/db|DB|Database|HaruDatabase|postgres|pglite|pool|db\.|\.transaction\(" packages/db/src packages/db/test 2>/dev/null | head -200 || true
echo "== deterministic node pipe flush comparison for direct stderr + exit behavior (sandbox only) =="
node - <<'JS'
function run(n) {
const p = require('child_process').spawn(process.execPath, ['-e', `
const stderr = process.stderr;
stderr._type = 'write';
console.error('${n}: queued');
process.stderr._type = 'none';
${n === 'exitCode' ? 'process.exitCode = 1' : 'process.exit(1)'}
`], { stdio: ['ignore', 'ignore', 'pipe'] });
let out = '';
p.stderr.on('data', (chunk) => out += chunk);
p.on('close', (code) => console.log(`${n}: childClose=${code}, stderrBytes=${out.length}, stderrText=${JSON.stringify(out.replace(/^.*?queued\n/g, ''))}`));
}
run('exit');
run('exitCode');
JSRepository: arkorlab/haru
Length of output: 7311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deterministic stdout flush comparison for direct stdout + exit behavior =="
node - <<'JS'
function run(n) {
const p = require('child_process').spawn(process.execPath, ['-e', `
const stdout = process.stdout;
console.log('${n}: queued');
${n === 'exitCode' ? 'process.exitCode = 1' : 'process.exit(1)'}
`], { stdio: ['ignore', 'pipe', 'pipe'] });
let out = '';
p.stdout.on('data', (chunk) => out += chunk);
p.on('close', (code) => console.log(`${n}: childClose=${code}, stdoutBytes=${out.length}, stdoutText=${JSON.stringify(out.replace(/^.*?queued\n/g, ''))}`));
}
run('exit');
run('exit');
run('exitCode');
run('exitCode');
JS
echo "== direct Node stderr write test under a pipe =="
node - <<'JS'
const p = require('child_process').spawn(process.execPath, ['-e', `
const chunk = Buffer.alloc(128 * 1024, Buffer.from("stderr ", "utf8"));
process.stderr.write(chunk);
process.exit(1);
`], { stdio: ['ignore', 'ignore', 'pipe'] });
let bytes = 0;
p.stderr.on('data', (data) => bytes += data.length);
p.on('close', (code) => console.log(`stdoutBytes=${bytes}, childClose=${code}`));
JS
echo "== direct Node console.log write test under a pipe =="
node - <<'JS'
const p = require('child_process').spawn(process.execPath, ['-e', `
const chunk = Buffer.alloc(128 * 1024, Buffer.from("stdout ", "utf8"));
process.stdout.write(chunk);
process.exit(1);
`], { stdio: ['ignore', 'pipe', 'pipe'] });
let bytes = 0;
p.stdout.on('data', (data) => bytes += data.length);
p.on('close', (code) => console.log(`stdoutBytes=${bytes}, childClose=${code}`));
JSRepository: arkorlab/haru
Length of output: 606
Exit the script only after closing the database client.
seed.ts creates the DB client in main(), but the only errors after that point call process.exit(1) and can skip cleanup. Close the client before exiting, or otherwise ensure cleanup cannot be bypassed by an early process.exit().
🤖 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 `@packages/db/src/seed.ts` around lines 66 - 67, Update main() error handling
so the database client is closed before any process.exit(1) call, including
errors occurring after client creation. Ensure cleanup runs reliably before
exiting while preserving the existing formatted error logging.
| // `#`-prefixed line inside a fenced code block (e.g. a shell comment) is | ||
| // not a heading at all, so track fence state and ignore it. | ||
| let isInFence = false; | ||
| let isEnded = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not suppress the empty-section error when a heading terminates the section.
If the section contains no variables and is followed by ###, ##, or #, isEnded is true, so this condition returns an empty set instead of throwing. Check documented.size === 0 unconditionally and add an empty-section fixture.
Proposed fix
- let isEnded = false;
const body = lines.slice(start + 1);
...
- isEnded = true;
break;
...
- if (!isEnded && documented.size === 0) {
+ if (documented.size === 0) {Also applies to: 55-55, 74-77
🤖 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 `@services/haru-server/src/environment-contract.test.ts` at line 41, Update the
empty-section validation in the relevant environment-contract test fixtures so
it checks documented.size === 0 regardless of isEnded, preserving the expected
error when a heading terminates an otherwise empty section. Add an empty-section
fixture covering termination by ###, ##, or # and apply the same correction to
the other referenced cases.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1c377e5bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return ( | ||
| name.endsWith(".json") && | ||
| name !== "package.json" && | ||
| !name.startsWith("tsconfig") | ||
| ); |
There was a problem hiding this comment.
Because isScannable accepts only the listed source/document extensions and selected JSON, every .txt file is excluded rather than only the two policy files. If a future seed, layout, documentation, or other shipped text file contains a forbidden workload identifier, the new gate remains green, despite the repository contract making those two governed files the sole exception; exclude those exact paths and scan other text files.
AGENTS.md reference: AGENTS.md:L217-L224
Useful? React with 👍 / 👎.
| function readReadme(): string { | ||
| return readFileSync( | ||
| fileURLToPath(new URL("../../../README.md", import.meta.url)), | ||
| "utf8", | ||
| ); |
There was a problem hiding this comment.
This drift gate reads only README.md, so adding or removing a server environment variable while updating the English table alone passes CI even if README.ja.md retains a stale contract. Since these documents are required to remain paired, run the same schema-key comparison against the Japanese environment table as well.
AGENTS.md reference: AGENTS.md:L272-L278
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/src/publishability.test.ts">
<violation number="1" location="packages/db/src/publishability.test.ts:260">
P3: The newly extracted `violationsInText` function implements the `publishability-allow` escape hatch, but no test case verifies that a line containing this marker is actually suppressed while the same content without the marker is reported. Adding a direct regression case would pin this behavior and prevent silent breakage of the escape hatch.</violation>
<violation number="2" location="packages/db/src/publishability.test.ts:263">
P2: The publishability test now fails on its own source: the added `qwen` example in this comment is matched by the denylist, while the scanner recursively includes this `.ts` file. Rephrasing the example without a literal denylisted identifier keeps the per-branch explanation without making the gate fail.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| it("detects every sampled identifier (per-token coverage)", () => { | ||
| // The rules are large alternations, so "the pattern matches the policy | ||
| // text" stays true even when individual alternatives are deleted. Each | ||
| // sample pins ONE branch: drop `|\\bqwen\\b` and this fails, where the |
There was a problem hiding this comment.
P2: The publishability test now fails on its own source: the added qwen example in this comment is matched by the denylist, while the scanner recursively includes this .ts file. Rephrasing the example without a literal denylisted identifier keeps the per-branch explanation without making the gate fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/src/publishability.test.ts, line 263:
<comment>The publishability test now fails on its own source: the added `qwen` example in this comment is matched by the denylist, while the scanner recursively includes this `.ts` file. Rephrasing the example without a literal denylisted identifier keeps the per-branch explanation without making the gate fail.</comment>
<file context>
@@ -122,33 +205,106 @@ describe("publishability", () => {
+ it("detects every sampled identifier (per-token coverage)", () => {
+ // The rules are large alternations, so "the pattern matches the policy
+ // text" stays true even when individual alternatives are deleted. Each
+ // sample pins ONE branch: drop `|\\bqwen\\b` and this fails, where the
+ // per-rule check above would not.
+ const samples = parseSamples(readFileSync(SAMPLES_PATH, "utf8"));
</file context>
| // sample pins ONE branch: drop `|\\bqwen\\b` and this fails, where the | |
| // sample pins ONE branch: drop an individual alternative and this fails, where the |
| expect(rule?.pattern.test('const a = "TEST-ACCEL";')).toBe(true); | ||
| }); | ||
|
|
||
| it("detects every sampled identifier (per-token coverage)", () => { |
There was a problem hiding this comment.
P3: The newly extracted violationsInText function implements the publishability-allow escape hatch, but no test case verifies that a line containing this marker is actually suppressed while the same content without the marker is reported. Adding a direct regression case would pin this behavior and prevent silent breakage of the escape hatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/src/publishability.test.ts, line 260:
<comment>The newly extracted `violationsInText` function implements the `publishability-allow` escape hatch, but no test case verifies that a line containing this marker is actually suppressed while the same content without the marker is reported. Adding a direct regression case would pin this behavior and prevent silent breakage of the escape hatch.</comment>
<file context>
@@ -122,33 +205,106 @@ describe("publishability", () => {
+ expect(rule?.pattern.test('const a = "TEST-ACCEL";')).toBe(true);
+ });
+
+ it("detects every sampled identifier (per-token coverage)", () => {
+ // The rules are large alternations, so "the pattern matches the policy
+ // text" stays true even when individual alternatives are deleted. Each
</file context>
Adversarial self-review of the gate rework found the gate could still be weakened without any test noticing: - The per-rule coverage check used `violation.includes(label)` on a formatted string, so an empty label (which the parser accepted) matched every violation, and a label that is a substring of another rule's rode on that rule's hits - either way a dead pattern looked covered. Return structured violations and compare labels exactly; reject an empty label at parse time. - The sample floor (`> 20` against 41 samples) licensed deleting twenty branch/sample pairs before anything failed. Pin the exact count so a deletion fails loudly and an addition is a visible one-line diff. - A comment in the gate's own source named a model family, which the rule it documents forbids everywhere except the two .txt data files; it escaped detection only because a regex escape ate the word boundary. All four review findings were reproduced before fixing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 992f92432d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "dist", | ||
| "coverage", | ||
| ".turbo", | ||
| "drizzle", |
There was a problem hiding this comment.
When a committed migration contains a forbidden identifier, this directory-level exclusion drops the entire packages/db/drizzle tree before extension filtering, so the gate remains green even though those SQL files ship and are replayed by deployments and tests. Scan .sql migrations and reserve exclusions for the two explicitly sanctioned policy data files.
AGENTS.md reference: AGENTS.md:L217-L222
Useful? React with 👍 / 👎.
|
Read this PR locally and worked through the 12 unresolved threads. Nine sit on Real, and all one bug: the gate's scope is narrower than the rule it enforcesAGENTS.md states the contract precisely - the gate scans "the packages/services/.github trees plus the root-level files", and its "ONE sanctioned exception" is the governed policy pair. The implementation has four exclusions, not one:
The consequence is the one outcome Suggested fix, verified locally: make Checked in both directions: with the patch, planting The self-assertion test needed updating alongside, and the replacement is stronger: instead of asserting Also real:
|
2 participants
Summary
Repo-wide adversarial audit of the control plane, drivers, and supervisor,
followed by the fixes and guardrails it surfaced. Every documented invariant
was probed against the code; the result is a small set of correctness fixes
plus automated gates for invariants that were previously enforced by review
alone. A follow-up code review of these changes (and two rounds of adversarial
self-review) is folded in.
No behavioral change to the data/control paths beyond the fixes below; the bulk
is defensive hardening and tests.
Correctness fixes
and service YAML with PyYAML (YAML 1.1); the default 1.2 output emitted
off/no/12:34:56-style string envs as plain scalars that a 1.1 loaderreinterprets as booleans/base-60 ints. Serialize through a shared
stringifySkyYamlso the schema choice lives in one place.joinUrlfails closed on base-path escape. A..segment inpathcouldclimb above the base path prefix
joinUrlexists to preserve, without movingthe origin (so the origin backstop missed it). Added a segment-boundary prefix
backstop.
checked only against the tick's in-memory snapshot; a concurrent reconciler
that recovered-then-re-degraded the active could escalate before the current
degraded period reached the grace. The guard now rides inside the escalation
UPDATE against the live
stateUpdatedAt.escalateDomainIfFleetIdletakes a named budgets object. Its two trailingduration params (which default to different values, 30s vs 60s) were adjacent
positional numbers a caller could transpose undetectably; they are now a
Pick<FleetPolicy, ...>object.db:seedexits cleanly on a bad layout file instead of a raw stack trace.Guardrails (invariants that had no automated enforcement)
db.transaction()closed at the type.transactionisOmitted fromHaruDatabaseso an interactive-transaction call (which throws at runtime onlyon Neon HTTP, but passes on the PGlite/Postgres test lanes) is now a compile
error; a lint rule remains as a secondary guard for a raw drizzle handle.
haru-server environmenttablemust match
serverEnvironmentSchema(the JSON Schemas and migrations werealready drift-gated; this prose contract was not).
denylist of specific GPU/LLM model names (the "workloads are pure data" rule
was human-reviewed only). Includes a positive control and a per-line
publishability-allowescape hatch.switch_activevstarget_not_routedrace test on the Postgreslane, exercising the block-then-unblock interleave the
routingCommittedcolumn guard exists for.
Docs
KNOWN_ISSUES.md/.ja.md,AGENTS.md, and the README env contract kept insync (EN/JA pairs updated together).
Testing
pnpm build && pnpm typecheck && pnpm lint && pnpm format:checkall green;pnpm test= 359 tests passing across all 7 packages;pnpm install --frozen-lockfileconsistent. The type-level transaction guard and both newgates were verified with negative probes (a planted violation fails as
expected).
Summary by cubic
Repo-wide adversarial audit fixing correctness gaps and adding invariant guardrails across control plane, drivers, and supervisor; also reinforces the publishability gate to prevent silent weakening of its checks.
Bug Fixes
stringifySkyYamlso values likeoff/12:34:56stay strings for PyYAML; removed deadyamldep in@haru/driver-skyserve.joinUrlnow rejects paths that escape the base prefix via..(segment‑boundary check) in@haru/protocol.stateUpdatedAtto close the concurrent‑reconciler race (@haru/db,haru-server).escalateDomainIfFleetIdlenow takes a named budgets object (Pick<FleetPolicy, ...>) to prevent positional swaps.db:seedexits cleanly with a single‑line error; extractedformatSeedErrorand added tests.Guardrails
transactionfromHaruDatabaseand lint‑bandb.transaction(); enforce single‑statement CAS transitions.serverEnvironmentSchema; walker ignores fenced code, stops at same‑or‑higher headings, and throws on unterminated fences.publishability-denylist.txt+ per‑tokenpublishability-samples.txt), strict loader fails on malformed/empty/empty‑label rules, structured violations use exact label matching, exact sample count pinned, scans all shipped modules (.ts/.mts/.cts/.mjs/.cjs/.js), docs (.md/.yaml/.yml), and JSON acrosspackages/,services/,.github/, and root files; policy/samples are exempt from scanning but used as end‑to‑end positives; broadened GPU/LLM patterns; per‑linepublishability-allowescape; scope limits documented inKNOWN_ISSUES(EN/JA).switch_activevstarget_not_routedasserts mutual exclusion under real lock contention.Written for commit 992f924. Summary will update on new commits.
Summary by CodeRabbit