fix(W4 sweep): red suite, an overstated type-safety verdict, and one-command start - #154
Merged
Conversation
Portable skill: PDF guidelines -> requirements inventory -> three-way traceability matrix (requirement <-> Linear ticket <-> code file:line), behavioral verification gating VERIFIED, gaps handoff for PM agent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pm_skill config field, init auto-detection, phase-5 invocation with passive fallback when no PM skill exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lignment) 10 tasks: skill files with full content (config template, inventory format, report format, SKILL.md), discoverability check, Ship shakedown (init/extract/baseline), determinism check, skill-audit pass + memory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review caught the plan's Task 2 content block contradicting the spec's error-handling rule (stop-and-ask on PDF hash mismatch). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…per, quote verification) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
54 requirements extracted from GFA_Week_4_ShipShape_Updated.pdf with stable W4-R<n> IDs and verbatim quotes. Includes the pdf_tool text cache (source-W4.md) that every quote was mechanically verified against, plus the config's filled sha256 and pdf_tool backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
54 active requirements traced to code with file:line evidence: 2 VERIFIED, 41 IMPLEMENTED-UNVERIFIED, 10 PARTIAL, 1 ASSUMED, 0 MISSING. Behavioral verification run by the controller: seed floors met (500 docs / 255 issues / 20 users / 35 sprints, after db:seed + audit/seed-augment.ts); Render deployment live (HTTP 200); pnpm type-check green. Two findings the sweep turned up by re-measuring rather than trusting a claim: docs/IMPROVEMENTS.md records the type-safety target as met, but the audit's own count.sh gives 1987 tracked violations against a 1535 baseline (+29%, wrong direction); and pnpm test exits 1 on two migrationRunner assertions that compare a Postgres-ordered query against a JS sort. Ticket mapping is BLOCKED — the Linear connector is unauthorized, recorded as a blocked dimension rather than as absent tickets. Config's code_roots widened beyond application source: this doc set's acceptance evidence lives mostly in audit/, terraform/, .github/ and repo-root docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Regenerated REPORT.md/gaps.md/matrix.baseline.json from corrected trace
sources — the artifacts are generated, so every fix landed upstream in the
cluster JSONs, the verification results, or the two render scripts.
I1 W4-R47's evidence had zero valid citations: `.git/refs (branch listing):1`
is not a file, and neither merge-commit subject appears in CHANGES.md
(line 10769 is the TRO-248 heading). Replaced with three CHANGES.md lines
that genuinely name the branch each improvement landed on (10763, 14335,
14357); the git-history evidence moved into notes with verified SHAs
(1474cb1, a1ad4f3, b35b817, 1ed03a4, 505e542, 1e4b68f, af056fa, dbae2af)
and the branch list. Verdict unchanged.
I2 Disclosed that the sweep wrote to the developer's database: W4-R13's seed
run targeted ship_standup, `pnpm test` then truncated 15 tables via
api/src/test/setup.ts:93-98, and it was re-seeded afterwards. Added a hard
rule to the skill requiring a throwaway database or an explicit report.
I4 Summary now leads with the two urgent findings — type-safety target unmet
(1987 vs 1535, +29%) while docs/IMPROVEMENTS.md records it met, and a red
suite at HEAD — instead of the blocked ticket dimension.
I5 New "Coverage and limitations" block right after the Summary: e2e never
ran, ticket mapping blocked, the database mutation, 41/54 rows statically
traced only, and which dirty paths are actually cited.
I4b "Verification performed" now tables every command run with its real
result, including pnpm test, pnpm type-check, count.sh, and the e2e suite
marked NOT RUN. Sourced from a new `commands_run` matrix field.
D1 report-format.md documents ticket_mapping, needs_ruling, commands_run,
dirty_paths, suggested_scope, and PDF page citations; blocked rows now
carry tickets:["BLOCKED"] so the matrix distinguishes "never checked" from
"checked, none found". SKILL.md reconciled to match.
D2 gaps.md restores the documented "Suggested scope" field — all 10 entries,
derived from each row's notes and pm-triage.md.
M1 api/vitest.config.ts:24 -> :27 (the coverage block). Dropped the
unreproducible memory-bank/activeContext.md:25 citation from W4-R35 and
moved the claim into notes with the reason. Matrix records dirty_paths.
M2 Removed both self-citations into the excluded audit/requirements root
(source-W4.md:332, inventory.md:6); replaced with the source PDF's
p.12 and p.7.
M3 Rewrote W4-R10/R33/R34/R35/R37/R51 notes into third person, dropping
"DEVIATING from the dispatch instruction", "this cluster", "the
controller", and the duplicated 1987 recount. Facts preserved.
M4 Corrected the W4-R10 accusation: docs/IMPROVEMENTS.md:27-28 marks its
inference explicitly and prints 1535 -> 1778 "Up 243" openly. The verdict
(PARTIAL) and the number (+29%) stand; the characterisation now says the
claim is disclosed and it is the literal threshold that is unmet. Same
correction applied in pm-triage.md.
M6 requirements.config.yaml drops `.` from code_roots (it admitted the whole
repo and made every other root redundant) in favour of the workspace
packages plus the seven repo-root files the requirements actually cite.
Also found and fixed while re-verifying every citation: CHANGES.md:9765 ->
:9785 with the quote made verbatim (W4-R10), and an off-by-one line reference
in a W4-R50 note. No verdict changed; 54 rows, acceptance check green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ruling I-01 (Troy, 2026-08-08): W4-R26's "navigate to terraform/" means the local-provider and Render exercise directories the same passage asks you to author, not the pre-existing AWS root. W4-R26 moves ASSUMED -> IMPLEMENTED-UNVERIFIED with interpretation I-01; needs_ruling is now empty and no verdict in the matrix is contingent. REPORT.md gains an "Interpretation rulings applied" section so a reader can see which verdicts rest on a judgement call. The review flagged that the sweep skipped its own ambiguity protocol here: only one ambiguity surfaced, far under the ~5 flood cap, so the skill required asking rather than emitting ASSUMED. Asked and settled. Also moves the generator into audit/requirements/pipeline/ (cluster traces, verification results, three scripts, README). REPORT.md and gaps.md were previously derivable only from a session scratch directory, so the baseline stopped being reproducible once that session ended — which an audit demanding reproducible before/after measurement should not ship. Verified: re-running the committed pipeline reproduces all 54 verdicts identically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pm-triage.md said "the 41 IMPLEMENTED-UNVERIFIED requirements"; applying
ruling I-01 moved W4-R26 into that tier, so it is 42. A wrong count in a
deliverable is the same defect class this sweep exists to find.
Also rewrites a W4-R33 evidence note that still addressed the fan-out rather
than the reader ("not independently deep-traced by this cluster").
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d, scoped Phase 2 was BLOCKED on an unauthorized Linear connector; it is authorized now, so the sweep's third dimension is live. 33 of 54 requirements have covering tickets, 21 have none, and 9 in-scope tickets map to no requirement. Scoped by Linear PROJECT, not by issue-number range, and that distinction is the finding. The TRO team is a personal catch-all spanning six projects whose issue numbers interleave: TRO-250..275 are Clavira Pilot Readiness, TRO-312..365 mostly FleetGraph (Week 5 — same repo, different assignment). Sweeping the team unscoped produced 88 orphans, ~80 of them simply other products' work. Scoped to "ShipShape Audit Remediation" (123 issues) it is 9, and those 9 are real signal: DB-11/DB-12, the CodeQL security tickets, and two TF-7 follow-ups did work this brief never asked for. config's tickets.project was null and is now set — the field existed for exactly this and going without it would have made orphan detection worse than useless. The 21 unticketed requirements are mostly process deliverables (the audit report itself, commit discipline, the submission artifacts) done without tickets rather than left undone; W4-R38 (dependency pinning) is the one that looks like a genuine uncovered gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The triage was written while Linear was unauthorized and said so. Records what the live mapping adds: W4-R38 is the only gap with no ticket covering it, and W4-R10/R33/R35 have tickets that name their own gaps (TRO-304, TRO-305). No disposition changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Phase 2 was blocked when the first triage ran, so ticket coverage never went through the scope gate. It has now. Rulings: none of the 21 unticketed requirements and none of the 9 orphan tickets imply work. "No ticket" is a defect only when the requirement is unsatisfied AND untracked — which is true of exactly two (W4-R35, W4-R38), and both are already "fix now", so a tracker entry would be overhead rather than coordination. Conditional: ticket them if they are not fixed before submission. The gate did surface something bigger than either list. The project defines done as "compare-mode measurement proves the delta AND the full suite still passes", and at HEAD pnpm test exits 1 while only 3 of 8 categories have a compare-* artifact. So 121 tickets are marked Done against a bar that does not currently hold — the same defect class as the IMPROVEMENTS.md claim, and invisible until the ticket dimension existed. Fixing the red suite repairs the clause for all 121 at once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-178) recordedVersions() compared a Postgres-ORDER-BY'd list against a JS-.sort()'d expectation. The two collations disagree on 020_document_associations vs 020b_sprint_assignee_ids (Postgres orders 020b first, JS orders 020_ first), so both real-migration-set assertions failed against a runner that had actually applied all 46 migrations correctly. Fix: sort recordedVersions()'s own result with JS .sort() before returning it, so both sides of every comparison use the same collation. The comparison itself is unchanged — still a strict, ordered toEqual, not a count or arrayContaining — preserving the DB-1 regression guarantee (the runner must apply every migration or fail). Proof: red on unfixed code (observed AssertionError with the 020/020b pair transposed), green after the fix (7/7, isolated), then re-proved red by temporarily filtering 020b_sprint_assignee_ids.sql out of runPendingMigrations's file list — both assertions failed correctly (44 vs 45 versions, 020b missing) — before reverting the sabotage back to a clean diff and confirming green again.
…ed total docs/IMPROVEMENTS.md §1 recorded W4-R10 (eliminate 25% of the 1535 tracked type-safety violations) as met, via a sum of controlled per-ticket diffs. Re-running the audit's own instrument at HEAD (bash ~/.claude/skills/type-safety-audit/scripts/count.sh web api shared) gives 1987 tracked violations (any 50 + as 1882 + non-null 47 + ts-ignore 8) against the 1535 baseline: +452 / +29%, where the requirement needs -25% (~-384). The tracked total has never measured below baseline at any point this sprint (1535 -> 1778 -> 1987). This was not a concealed claim — the document's own lines 27-28 already disclosed the sum-of-diffs method and the adjacent table already showed the total rising. The defect was narrower: the verdict field said "met" against a requirement whose literal threshold is the tracked total, not the per-ticket sum. Restated the verdict as not met against that threshold, kept the genuine per-ticket wins (any 102->50, req.userId!/req.workspaceId! 236->0, both reconfirmed this session), and added this session's full recount with its formula and source. No regression test: a verdict correction to a static-analysis doc has no observable behavior to assert on.
…able (W4-R42) ./start.sh and scripts/dev.sh stopped at ensureDatabase.ts's actionable "Postgres unreachable" message, requiring the user to start Postgres by hand before retrying. W4-R42 requires one command to bring up the full composed system with no manual setup step beyond installing dependencies. scripts/dev.sh (both start.sh and `pnpm dev` funnel through it) now checks reachability before resolving api/.env.local, and — only when neither an explicit DATABASE_URL nor an existing api/.env.local pinned a database, so nothing configured is ever silently redirected — brings up docker-compose.local.yml's postgres service, waits for it to report healthy, and repoints DATABASE_URL at it before continuing into the existing ensureDatabase/migrate/verify/seed flow unchanged. The compose project name is pinned to `-p ship` rather than left directory-derived, so every worktree reuses the one shared Postgres container this repo's factory tooling already runs everything against, instead of colliding on docker-compose.local.yml's fixed 5433:5432 host port mapping. Falls through unchanged to ensureDatabase.ts's own message when Docker is unavailable, the compose file is missing, or the compose command itself fails -- api/src/db/ensureDatabase.ts is untouched. Extracted the reachability check into api/src/db/postgresReachable.ts (typed, tested) rather than an inline node -e snippet, matching the ensureDatabase.ts/migrate.ts pattern already used by this script. Verified twice, genuinely end-to-end: stopped the real Postgres container, removed api/.env.local, unset DATABASE_URL, ran the real ./scripts/dev.sh -- it bootstrapped Postgres, created the database, applied all 46 migrations, seeded, and brought up the API/web/agent dev servers successfully. Cleaned up afterward (throwaway database dropped, real api/.env.local and ship_wt_w4_sweepa confirmed intact).
The test exercises isPostgresReachable() with real sockets (genuine coverage of the probe), but not postgresReachable.ts's CLI wrapper (main(), the argv/exit-code mapping scripts/dev.sh actually invokes via `npx tsx ... | $?`). An inverted exit code there would stay green under this suite. Flagged in response to a direct question about it rather than left implicit in the original entry.
|
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:
WalkthroughThis PR adds a W4 requirements-audit system with configuration, evidence, generation pipelines, reports, and triage records. It also adds TCP-based Postgres reachability checks, Docker-based local startup, socket tests, and migration-version sorting. ChangesW4 audit and database workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DevScript
participant ReachabilityCheck
participant DockerCompose
participant Postgres
DevScript->>ReachabilityCheck: Check the default database URL
ReachabilityCheck->>Postgres: Open a timed TCP socket
Postgres-->>ReachabilityCheck: Return connection result
DevScript->>DockerCompose: Start postgres when unreachable
DockerCompose->>Postgres: Wait for healthy status
DevScript->>Postgres: Use localhost:5433 when healthy
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The prior test covered isPostgresReachable() but not main()'s CLI wrapper -- the actual contract scripts/dev.sh depends on (`npx tsx src/db/postgresReachable.ts "$URL"`, branching on `$?`). An inverted exit-code mapping there would have stayed green under the existing suite while one-command start silently broke: the "test that cannot fail when the thing breaks" shape this repo's own audit found 68 times. Added a second describe block that spawns the real CLI as a subprocess (execFile, from api/, matching scripts/dev.sh's exact invocation and cwd) and asserts on the numeric exit code recovered from the rejection -- 0 when reachable, non-zero when not -- not on stdout text. Confirmed red first, on the specific regression this closes: temporarily inverted process.exit(reachable ? 0 : 1) to process.exit(reachable ? 1 : 0) in main(). The two new CLI cases failed (AssertionError: expected 1 to be +0 / expected +0 not to be +0) while the four isPostgresReachable() cases stayed green -- confirming the failure was isolated to main()'s mapping, not the underlying probe. Reverted (git diff --stat back to empty) and re-ran: 6/6 green. Updates the CHANGES.md caveat from the previous commit to describe what's now covered, rather than leaving a stale "not covered" note.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@api/src/db/__tests__/postgresReachable.test.ts`:
- Around line 53-60: Update the default-port test for isPostgresReachable to
mock net.createConnection instead of making a real connection to 127.0.0.1.
Configure the mock to exercise the refused-connection result, assert that it was
called with options containing port: 5432, and preserve the expected false
outcome.
In `@audit/requirements/gaps.md`:
- Around line 7-14: Regenerate the W4-R10 handoff to reflect that
docs/IMPROVEMENTS.md already records the tracked-total result as NOT MET. In
audit/requirements/gaps.md:7-14, update the missing-work description and
suggested scope while keeping W4-R10 PARTIAL if the tracked-total gap remains;
in audit/requirements/pm-triage.md:16-56, remove the completed documentation
correction from mandatory disposition and retain only open remediation work.
In `@audit/requirements/pipeline/acceptance-check.py`:
- Around line 2-4: Update the module-level input loading around m, inv, and gaps
to derive the repository root from Path(__file__) rather than the current
working directory, then construct each artifact path relative to that root so
the script works when launched from any directory.
- Around line 5-9: Update the requirement parsing around active, retired, and
missing_rows to inspect each requirement section, recognize both plain and
Markdown-formatted retired statuses, and exclude only IDs explicitly marked
retired from active. Remove the retired-count fallback so the assertion always
fails whenever missing_rows contains any active requirement ID.
- Around line 9-16: Replace every acceptance check assert in the requirements
validation loop—including the missing_rows, VERIFIED, MISSING, and ASSUMED
checks—with explicit failure handling that remains active under python -O. Raise
an exception or aggregate violations and terminate with a nonzero exit status,
while preserving the existing validation messages and successful OK path.
In `@audit/requirements/pipeline/cluster-b.json`:
- Around line 14-47: The audit pipeline inputs still contain pre-fix evidence.
In audit/requirements/pipeline/cluster-b.json lines 14-47, replace the “met” and
1,778-violation references with the corrected “not met” verdict and 1,987
recount, while preserving accurate per-ticket evidence. In lines 371-374, remove
the completed migration-sort work from suggested_scope and update the
verification status accordingly.
In `@audit/requirements/pipeline/cluster-e.json`:
- Around line 5-47: Synchronize the audit outputs with the R35 and R42 fixes:
verify the migration test’s JavaScript sorting and scripts/dev.sh’s local
Postgres startup/wait behavior, then update
audit/requirements/pipeline/cluster-e.json (lines 5-47) to reflect the resolved
requirements. Remove the stale hard-coded red-suite text and count adjustment in
audit/requirements/pipeline/write-report.py (lines 83-96), regenerate
audit/requirements/gaps.md (lines 43-77), and retire the W4-R35/W4-R42 actions
and dispositions in audit/requirements/pm-triage.md (lines 63-94 and 159-172).
In `@audit/requirements/pipeline/verification-results.json`:
- Around line 11-13: Regenerate the audit from corrected implementation-check
evidence: update audit/requirements/pipeline/verification-results.json lines
11-13 with the current W4-R35 passing migration-test result, then regenerate
audit/requirements/REPORT.md lines 11 and 104-107. Also update W4-R42 source
evidence and regenerate audit/requirements/REPORT.md lines 119-122 to reflect
Dockerized Postgres bootstrap behavior, removing superseded claims about the
type-safety target and local startup.
In `@docs/superpowers/specs/2026-08-08-requirements-audit-skill-design.md`:
- Around line 79-83: Update the verification configuration example to move the
application URL from verify.app_url to the active verify_urls.app contract,
while keeping the test and e2e command entries under verify unchanged.
🪄 Autofix
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: ee67b362-be42-42dd-a897-e71d7f44b200
📒 Files selected for processing (30)
CHANGES.mdapi/src/db/__tests__/migrationRunner.test.tsapi/src/db/__tests__/postgresReachable.test.tsapi/src/db/postgresReachable.tsaudit/requirements.config.yamlaudit/requirements/REPORT.mdaudit/requirements/gaps.mdaudit/requirements/interpretations.mdaudit/requirements/inventory.mdaudit/requirements/matrix.baseline.jsonaudit/requirements/pipeline/README.mdaudit/requirements/pipeline/acceptance-check.pyaudit/requirements/pipeline/cluster-a.jsonaudit/requirements/pipeline/cluster-b.jsonaudit/requirements/pipeline/cluster-c.jsonaudit/requirements/pipeline/cluster-d.jsonaudit/requirements/pipeline/cluster-e.jsonaudit/requirements/pipeline/cluster-f.jsonaudit/requirements/pipeline/merge-matrix.pyaudit/requirements/pipeline/tickets-map-1.jsonaudit/requirements/pipeline/tickets-map-2.jsonaudit/requirements/pipeline/tickets-ship.jsonaudit/requirements/pipeline/verification-results.jsonaudit/requirements/pipeline/write-report.pyaudit/requirements/pm-triage.mdaudit/requirements/source-W4.mddocs/IMPROVEMENTS.mddocs/superpowers/plans/2026-08-08-requirements-audit-skill.mddocs/superpowers/specs/2026-08-08-requirements-audit-skill-design.mdscripts/dev.sh
Comment on lines
+7
to
+14
| ### W4-R10 — PARTIAL | ||
| - **Quote:** "Eliminate 25% of type safety violations. Every fix must preserve existing functionality (all tests still pass). Superficial fixes do not count. Replacing any with unknown without proper type narrowing is not an improvement." | ||
| - **Source:** GFA_Week_4_ShipShape_Updated.pdf, p.3 | ||
| - **Meaning in code:** Post-improvement violation count ≤ 75% of baseline with meaningful types; suite green. | ||
| - **Tickets:** TRO-167, TRO-206, TRO-207, TRO-208, TRO-209, TRO-210, TRO-211, TRO-212, TRO-213, TRO-214, TRO-297, TRO-306 | ||
| - **What is missing:** docs/IMPROVEMENTS.md:27-28 records this category as "Verdict: met, by the sum of controlled per-ticket diffs — not by a live recount, which the tracked metric cannot support today", and the table directly beneath it prints the tracked total as 1535 -> 1778, "Up 243", openly. So the inference behind "met" is marked, not hidden; what fails is narrower and harder to argue away — the requirement's literal threshold ("post-improvement violation count <= 75% of baseline") is defined on the tracked total, and the tracked total has not been below 1535 at any point measured. The alternative accounting the document uses to reach "met" sums controlled per-ticket diffs: TS-1 (156) + TS-3 (19) + TS-4 (236 raw) = 411 >= 384 (IMPROVEMENTS.md:57). The underlying work is real and well-evidenced rather than fabricated — TS-1..TS-10 make narrowly scoped fixes with genuine before/after diffs (api explicit_any 78 -> 36; req.userId! / req.workspaceId! 236 -> 0). Two further records from the repo's own history point the same way: CHANGES.md:9785-9786 is the TS-4 ticket's admission that "a live \"current total vs. 1535\" snapshot cannot cleanly demonstrate the category's cumulative progress - unrelated development moves it in both directions", and CHANGES.md:9780 records that ticket's own live re-run at commit 42e60d9 already finding a tracked total of 1747 — above baseline — before its fix was applied. No compare artifact exists under audit/type-safety/ at all; only baseline.json and baseline.md. Re-measured at HEAD rather than accepted from the repo's own record. `bash ~/.claude/skills/type-safety-audit/scripts/count.sh <web> <api> <shared>` — the exact instrument audit/type-safety/baseline.md:14-18 prescribes — returned web 23 any / 659 as / 5 non-null / 3 ts-ignore, api 27 / 1212 / 42 / 5, shared 0 / 11 / 0 / 0. Summed by baseline.md:77's own formula (any + as + ! + ts-ignore, as-any not double-counted): any 50, as 1882, non-null 47, ts-ignore 8 = 1987 tracked violations, against the 1535 baseline recorded in audit/type-safety/baseline.json (metrics.violationsTotal). That is +452 (+29%) where the requirement asks for -25% (about -384 sites), so the literal threshold is not met at HEAD on 2026-08-08. The nuance the headline number hides: explicit `any` halved (102 -> 50), a real gain in the most meaningful sub-metric, and the rise is driven almost entirely by `as` assertions (1385 -> 1882) — a pattern baseline.md:62 itself documents as over-counting by 15-20% because it catches import/export aliases, comments and `as const`. The direction of the tracked total is not in doubt, but the tracked total is a noisy proxy. The other half of this requirement's acceptance evidence: `pnpm type-check` is green (exit 0, all 4 packages); `pnpm test` exits 1 with 2 failures (see W4-R35). | ||
| - **Suggested scope:** Two separable changes, and only the first is small. (a) Restate the verdict at docs/IMPROVEMENTS.md:24-28 as not met against the tracked total, keeping the real per-ticket wins and printing 1987 vs 1535 plainly — minutes of documentation work, and it stops a grader re-running count.sh from finding a success claim that the instrument contradicts. (b) Closing the gap itself is not small: it needs the `as` assertion growth (1385 -> 1882) genuinely reduced, or the baseline re-measured with the corrected pattern audit/type-safety/baseline.md:62 already documents — applied to the baseline as well as to HEAD, with both numbers published. | ||
| - **Existing partial evidence:** `audit/type-safety/baseline.json:32`, `docs/IMPROVEMENTS.md:24`, `docs/IMPROVEMENTS.md:35`, `docs/IMPROVEMENTS.md:28` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Regenerate the W4-R10 handoff after the documentation correction.
docs/IMPROVEMENTS.md now states NOT MET at lines 28-36. These handoff records still state that it claims met and direct work to correct that claim. This sends already-completed work back into triage and makes the audit artifacts disagree.
audit/requirements/gaps.md#L7-L14: update the missing-work description and suggested scope. Keep W4-R10PARTIALif the tracked-total gap remains.audit/requirements/pm-triage.md#L16-L56: remove the completed documentation correction from the mandatory disposition. Retain only open remediation work.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
📍 Affects 2 files
audit/requirements/gaps.md#L7-L14(this comment)audit/requirements/pm-triage.md#L16-L56
🤖 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 `@audit/requirements/gaps.md` around lines 7 - 14, Regenerate the W4-R10
handoff to reflect that docs/IMPROVEMENTS.md already records the tracked-total
result as NOT MET. In audit/requirements/gaps.md:7-14, update the missing-work
description and suggested scope while keeping W4-R10 PARTIAL if the
tracked-total gap remains; in audit/requirements/pm-triage.md:16-56, remove the
completed documentation correction from mandatory disposition and retain only
open remediation work.
Comment on lines
+14
to
+47
| "file": "docs/IMPROVEMENTS.md", | ||
| "line": 24, | ||
| "note": "Category 1 target quoted verbatim: eliminate 25% of 1535 tracked violations ~= 384 sites" | ||
| }, | ||
| { | ||
| "file": "docs/IMPROVEMENTS.md", | ||
| "line": 35, | ||
| "note": "repo's own before/after table: tracked total (count.sh, web+api+shared) baseline 1535 -> 1778 on commit 09a6895 (2026-07-31) -- an INCREASE, not the required decrease" | ||
| }, | ||
| { | ||
| "file": "docs/IMPROVEMENTS.md", | ||
| "line": 28, | ||
| "note": "repo claims 'Verdict: met, by the sum of controlled per-ticket diffs -- not by a live recount, which the tracked metric cannot support today'" | ||
| }, | ||
| { | ||
| "file": "docs/IMPROVEMENTS.md", | ||
| "line": 57, | ||
| "note": "alternative accounting: TS-1(156)+TS-3(19)+TS-4(236 raw)=411 >= 384, used to justify 'met' despite the aggregate not moving" | ||
| }, | ||
| { | ||
| "file": "CHANGES.md", | ||
| "line": 9785, | ||
| "note": "TS-4 ticket's own admission, verbatim: \"This means a live \\\"current total vs. 1535\\\" snapshot cannot cleanly demonstrate the category's cumulative progress - unrelated development moves it in both directions\" (sentence runs 9785-9786)" | ||
| }, | ||
| { | ||
| "file": "CHANGES.md", | ||
| "line": 9780, | ||
| "note": "TS-4 ticket's own live re-run at an earlier commit (42e60d9) already found tracked total 1747, higher than the 1535 baseline, before this ticket's fix was even applied" | ||
| } | ||
| ], | ||
| "suggested_verification": "pnpm --filter @ship/api exec tsc --noEmit && pnpm --filter @ship/web type-check && pnpm test -- confirms suite-green component only; does not change the numeric-target finding below", | ||
| "assumption": null, | ||
| "notes": "docs/IMPROVEMENTS.md:27-28 records this category as \"Verdict: met, by the sum of controlled per-ticket diffs \u2014 not by a live recount, which the tracked metric cannot support today\", and the table directly beneath it prints the tracked total as 1535 -> 1778, \"Up 243\", openly. So the inference behind \"met\" is marked, not hidden; what fails is narrower and harder to argue away \u2014 the requirement's literal threshold (\"post-improvement violation count <= 75% of baseline\") is defined on the tracked total, and the tracked total has not been below 1535 at any point measured. The alternative accounting the document uses to reach \"met\" sums controlled per-ticket diffs: TS-1 (156) + TS-3 (19) + TS-4 (236 raw) = 411 >= 384 (IMPROVEMENTS.md:57). The underlying work is real and well-evidenced rather than fabricated \u2014 TS-1..TS-10 make narrowly scoped fixes with genuine before/after diffs (api explicit_any 78 -> 36; req.userId! / req.workspaceId! 236 -> 0). Two further records from the repo's own history point the same way: CHANGES.md:9785-9786 is the TS-4 ticket's admission that \"a live \\\"current total vs. 1535\\\" snapshot cannot cleanly demonstrate the category's cumulative progress - unrelated development moves it in both directions\", and CHANGES.md:9780 records that ticket's own live re-run at commit 42e60d9 already finding a tracked total of 1747 \u2014 above baseline \u2014 before its fix was applied. No compare artifact exists under audit/type-safety/ at all; only baseline.json and baseline.md.", | ||
| "suggested_scope": "Two separable changes, and only the first is small. (a) Restate the verdict at docs/IMPROVEMENTS.md:24-28 as not met against the tracked total, keeping the real per-ticket wins and printing 1987 vs 1535 plainly \u2014 minutes of documentation work, and it stops a grader re-running count.sh from finding a success claim that the instrument contradicts. (b) Closing the gap itself is not small: it needs the `as` assertion growth (1385 -> 1882) genuinely reduced, or the baseline re-measured with the corrected pattern audit/type-safety/baseline.md:62 already documents \u2014 applied to the baseline as well as to HEAD, with both numbers published." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Regenerate the audit pipeline inputs after the W4-SWEEPA corrections.
The pipeline still contains pre-fix evidence. This can produce a report and PM handoff that contradict the corrected documentation and completed migration fix.
audit/requirements/pipeline/cluster-b.json#L14-L47: replace the old “met” and 1,778-violation citations with the corrected “not met” verdict and 1,987 recount.audit/requirements/pipeline/cluster-b.json#L371-L374: remove the completed migration-sort change fromsuggested_scopeand update the verification status.
📍 Affects 1 file
audit/requirements/pipeline/cluster-b.json#L14-L47(this comment)audit/requirements/pipeline/cluster-b.json#L371-L374
🤖 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 `@audit/requirements/pipeline/cluster-b.json` around lines 14 - 47, The audit
pipeline inputs still contain pre-fix evidence. In
audit/requirements/pipeline/cluster-b.json lines 14-47, replace the “met” and
1,778-violation references with the corrected “not met” verdict and 1,987
recount, while preserving accurate per-ticket evidence. In lines 371-374, remove
the completed migration-sort work from suggested_scope and update the
verification status accordingly.
Comment on lines
+5
to
+47
| "id": "W4-R42", | ||
| "verdict": "PARTIAL", | ||
| "evidence": [ | ||
| { | ||
| "file": "start.sh", | ||
| "line": 68, | ||
| "note": "start.sh execs scripts/dev.sh unchanged after a Node/pnpm preflight; it is the documented single entry point" | ||
| }, | ||
| { | ||
| "file": "scripts/dev.sh", | ||
| "line": 78, | ||
| "note": "dev.sh runs ensureDatabase.ts, migrate.ts, verifyMigrations.ts, seed.ts on every invocation (idempotent, self-healing)" | ||
| }, | ||
| { | ||
| "file": "api/src/db/ensureDatabase.ts", | ||
| "line": 53, | ||
| "note": "unreachableMessage() builds the exact text 'ERROR: Cannot reach PostgreSQL at HOST:PORT. Start it, then re-run ./start.sh...' — thrown from ensureDatabase() at line 93 when the initial client.connect() gets ECONNREFUSED/ENOTFOUND/ETIMEDOUT" | ||
| }, | ||
| { | ||
| "file": "api/src/db/ensureDatabase.ts", | ||
| "line": 93, | ||
| "note": "connection-error branch converts a raw pg connect failure into the actionable unreachableMessage and throws — set -e in dev.sh then stops the script before servers start" | ||
| }, | ||
| { | ||
| "file": "README.md", | ||
| "line": 37, | ||
| "note": "README has a 'Cold start (one command)' section documenting ./start.sh from a clean checkout" | ||
| }, | ||
| { | ||
| "file": "README.md", | ||
| "line": 55, | ||
| "note": "README explicitly says: 'No native Postgres installed? Bring up one of the two bundled Docker options first, then point start.sh at it with DATABASE_URL' — a manual pre-step, not part of the one command" | ||
| }, | ||
| { | ||
| "file": "README.md", | ||
| "line": 69, | ||
| "note": "'If Postgres isn't reachable at all, ./start.sh fails immediately and says exactly what to start' — the README itself documents that the single command does not start the database server" | ||
| } | ||
| ], | ||
| "suggested_verification": "On a machine with no Postgres server running anywhere (no native service, no docker-compose container), run `./start.sh` from a clean checkout and observe whether it succeeds unaided or requires a manual `docker compose ... up`/`brew services start postgresql` first.", | ||
| "assumption": null, | ||
| "notes": "PARTIAL. Everything downstream of an already-reachable Postgres server is genuinely one-command and self-healing: dependency install, shared build, database creation, migration, migration verification, seeding, dynamic port selection, and starting both servers — all idempotent, all in scripts/dev.sh, all documented in README's 'Cold start' section. But the script only creates/migrates/seeds a database ON a Postgres SERVER that is already running; it does not start the Postgres server process itself. From a truly clean checkout on a machine with no Postgres running (native service stopped, no Docker container up), `./start.sh` fails immediately with the exact message observed today ('ERROR: Cannot reach PostgreSQL at localhost:5433. Start it, then re-run ./start.sh' — produced by api/src/db/ensureDatabase.ts:53-61's unreachableMessage(), thrown at line 93) and requires a manual step (start a local Postgres service, or `docker compose -f docker-compose.local.yml up -d postgres`) before re-running — which is manual setup beyond 'installing dependencies' under a strict reading of the quote. The README is honest about this (README.md:55-70) rather than hiding it, so the documentation half of R42 is fully met; the 'single command boots app + database... without manual setup beyond installing dependencies' half is not, for the database-server-startup step specifically. No mock external services were found to be needed for local dev (the only external call, AWS SSM, is gated behind NODE_ENV=production in api/src/index.ts:15-17), so that clause of the quote is not in play.", | ||
| "suggested_scope": "About 20 lines in start.sh: when Postgres is unreachable and docker-compose.local.yml is present, offer to start that container and wait for it to become healthy, keeping the current message from api/src/db/ensureDatabase.ts:53-61 as the fallback. No application code, and the README already documents the limitation honestly, so only the script changes." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'migrationRunner.test.ts|postgresReachable.ts|dev.sh|start.sh' .
rg -n -C 4 \
'postgresReachable|docker compose|docker-compose|Cannot reach PostgreSQL|wait' \
start.sh scripts/dev.sh api/src/db 2>/dev/null || true
rg -n -C 4 \
'schema_migrations|ORDER BY version|expected.*sort|sort\\(' \
api/src/db/__tests__/migrationRunner.test.ts 2>/dev/null || true
rg -n -C 3 \
'W4-R35|W4-R42|suite is red|one-command start' \
audit/requirements docs/IMPROVEMENTS.mdRepository: troysatchell/ship
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration test ---'
sed -n '90,205p' api/src/db/__tests__/migrationRunner.test.ts
printf '%s\n' '--- startup implementation ---'
sed -n '1,155p' scripts/dev.sh
sed -n '1,110p' api/src/db/postgresReachable.ts
printf '%s\n' '--- audit generator and records ---'
sed -n '75,105p' audit/requirements/pipeline/write-report.py
sed -n '1,65p' audit/requirements/pipeline/cluster-e.json
sed -n '35,85p' audit/requirements/gaps.md
sed -n '55,100p' audit/requirements/pm-triage.md
sed -n '155,175p' audit/requirements/pm-triage.md
printf '%s\n' '--- relevant worktree changes ---'
git status --short
git diff --stat -- audit/requirements api/src/db scriptsRepository: troysatchell/ship
Length of output: 38128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import json
def text(path):
return Path(path).read_text()
dev = text("scripts/dev.sh")
migration = text("api/src/db/__tests__/migrationRunner.test.ts")
report = text("audit/requirements/pipeline/write-report.py")
cluster = json.loads(text("audit/requirements/pipeline/cluster-e.json"))
gaps = text("audit/requirements/gaps.md")
triage = text("audit/requirements/pm-triage.md")
checks = {
"startup probes reachability": "postgresReachable.ts" in dev,
"startup starts compose postgres": "docker compose -p ship -f docker-compose.local.yml up -d postgres" in dev,
"startup waits for health": "HEALTH" in dev and 'healthy' in dev,
"startup redirects to compose port": "localhost:5433" in dev,
"migration result uses JS sort": "return res.rows.map(r => r.version).sort();" in migration,
"report still hard-codes red suite": "The test suite is red at HEAD" in report,
"cluster still marks R42 partial": next(r["verdict"] for r in cluster["requirements"] if r["id"] == "W4-R42") == "PARTIAL",
"gaps still contain stale R35": "### W4-R35 — PARTIAL" in gaps,
"gaps still contain stale R42": "### W4-R42 — PARTIAL" in gaps,
"triage still lists R35 fix now": "| W4-R35 | **Fix now** |" in triage,
"triage still lists R42 fix now": "| W4-R42 | **Fix now** |" in triage,
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: troysatchell/ship
Length of output: 556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import json
def text(path):
return Path(path).read_text()
dev = text("scripts/dev.sh")
migration = text("api/src/db/__tests__/migrationRunner.test.ts")
report = text("audit/requirements/pipeline/write-report.py")
cluster = json.loads(text("audit/requirements/pipeline/cluster-e.json"))
gaps = text("audit/requirements/gaps.md")
triage = text("audit/requirements/pm-triage.md")
checks = {
"startup probes reachability": "postgresReachable.ts" in dev,
"startup starts compose postgres": "docker compose -p ship -f docker-compose.local.yml up -d postgres" in dev,
"startup waits for health": "HEALTH" in dev and "healthy" in dev,
"startup redirects to compose port": "localhost:5433" in dev,
"migration result uses JS sort": "return res.rows.map(r => r.version).sort();" in migration,
"report still hard-codes red suite": "The test suite is red at HEAD" in report,
"cluster still marks R42 partial": next(r["verdict"] for r in cluster["requirements"] if r["id"] == "W4-R42") == "PARTIAL",
"gaps still contain stale R35": "### W4-R35 — PARTIAL" in gaps,
"gaps still contain stale R42": "### W4-R42 — PARTIAL" in gaps,
"triage still lists R35 fix now": "| W4-R35 | **Fix now** |" in triage,
"triage still lists R42 fix now": "| W4-R42 | **Fix now** |" in triage,
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: troysatchell/ship
Length of output: 556
Synchronize the audit with the R35 and R42 fixes.
The migration test now applies JavaScript sorting, and scripts/dev.sh now starts and waits for the local Postgres container. Run the relevant checks, then update cluster-e.json, remove the stale hard-coded red-suite text and count adjustment in write-report.py, regenerate gaps.md, and retire the W4-R35/W4-R42 actions and dispositions in pm-triage.md.
📍 Affects 4 files
audit/requirements/pipeline/cluster-e.json#L5-L47(this comment)audit/requirements/pipeline/write-report.py#L83-L96audit/requirements/gaps.md#L43-L77audit/requirements/pm-triage.md#L63-L94audit/requirements/pm-triage.md#L159-L172
🤖 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 `@audit/requirements/pipeline/cluster-e.json` around lines 5 - 47, Synchronize
the audit outputs with the R35 and R42 fixes: verify the migration test’s
JavaScript sorting and scripts/dev.sh’s local Postgres startup/wait behavior,
then update audit/requirements/pipeline/cluster-e.json (lines 5-47) to reflect
the resolved requirements. Remove the stale hard-coded red-suite text and count
adjustment in audit/requirements/pipeline/write-report.py (lines 83-96),
regenerate audit/requirements/gaps.md (lines 43-77), and retire the
W4-R35/W4-R42 actions and dispositions in audit/requirements/pm-triage.md (lines
63-94 and 159-172).
Comment on lines
+11
to
+13
| "W4-R35": { | ||
| "verdict": "PARTIAL", | ||
| "notes": "Suite is NOT green at HEAD: `pnpm test` exits 1 with 830/832 tests passing and 2 failures, both in api/src/db/__tests__/migrationRunner.test.ts (lines 167 and 184). The cause was confirmed by direct observation rather than inferred: the test compares a Postgres-ordered result (`SELECT version FROM schema_migrations ORDER BY version`, line 115) against a JavaScript `[...expected].sort()`, and the two orderings genuinely disagree on the pair 020_document_associations / 020b_sprint_assignee_ids — Postgres returns 020b first, JavaScript `.sort()` returns 020_ first. An environment caveat that must not be dropped: an earlier run of the same command failed all 77 files with \"DATABASE_URL must be set\" because the shell lacked the env var; the 830/832 figure comes from the re-run with api/.env.local loaded. `pnpm type-check` is separately green (exit 0, all 4 packages). E2E (`pnpm test:e2e`) was NOT run this sweep — 600+ Playwright tests requiring the /e2e-test-runner protocol — so no claim is made about it either way." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Regenerate the audit after the corrected implementation checks.
The committed audit still describes superseded behavior. Line 13 records the migration sort failure as current, and merge-matrix.py applies this override to generated output. The PR objective states that the sort fix now passes. The report also still says docs/IMPROVEMENTS.md declares the type-safety target met and that local startup cannot boot Dockerized Postgres.
audit/requirements/pipeline/verification-results.json#L11-L13: replace the stale W4-R35 result with fresh test evidence.audit/requirements/REPORT.md#L11-L11: regenerate the summary from corrected source evidence.audit/requirements/REPORT.md#L104-L107: regenerate W4-R35 after updating its source verdict and evidence.audit/requirements/REPORT.md#L119-L122: update the W4-R42 source evidence and regenerate the report to reflect Docker Postgres bootstrap behavior.
📍 Affects 2 files
audit/requirements/pipeline/verification-results.json#L11-L13(this comment)audit/requirements/REPORT.md#L11-L11audit/requirements/REPORT.md#L104-L107audit/requirements/REPORT.md#L119-L122
🤖 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 `@audit/requirements/pipeline/verification-results.json` around lines 11 - 13,
Regenerate the audit from corrected implementation-check evidence: update
audit/requirements/pipeline/verification-results.json lines 11-13 with the
current W4-R35 passing migration-test result, then regenerate
audit/requirements/REPORT.md lines 11 and 104-107. Also update W4-R42 source
evidence and regenerate audit/requirements/REPORT.md lines 119-122 to reflect
Dockerized Postgres bootstrap behavior, removing superseded claims about the
type-safety target and local startup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@api/src/db/__tests__/postgresReachable.test.ts`:
- Around line 50-55: Update the catch block in the postgres reachability test to
verify error is non-null and an object before reading its code property, then
return only when code is numeric; otherwise rethrow the original error
unchanged.
🪄 Autofix
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: 67e5b75f-50aa-49ad-96f8-df3ba69ebb66
📒 Files selected for processing (2)
CHANGES.mdapi/src/db/__tests__/postgresReachable.test.ts
…e host CI (PR #154) failed with exactly one new case: postgresReachable.test.ts :: isPostgresReachable "defaults to port 5432 when the URL specifies none". It asserted isPostgresReachable('postgresql://127.0.0.1/whatever') resolved false, inferring "the default is 5432" from "nothing answered on 5432" -- true where this was written (local Postgres is on 5433), false on GitHub Actions, which runs its own Postgres service on 5432. The probe correctly returned true; the test failed because the assertion depended on the environment, not the code. Same defect class as item 1 (migrationRunner.test.ts's collation assumption) -- and the local gate could never have caught it, since it passed specifically because it ran somewhere the assumption held. Extracted the URL parsing isPostgresReachable did inline into a pure resolveHostPort(url): { host, port } | null (same default-port rule, no socket). Replaced the socket-probe "defaults to 5432" case with a new `describe('resolveHostPort', ...)` block that asserts directly on the parsed value -- deterministic on every machine, since it never opens a socket. Audited the rest of the file for the same shape: every other socket-touching case is either self-consistent (an ephemeral OS-assigned server + connecting to that exact port) or uses port 1, a guaranteed-closed well-known port and the same pattern ensureDatabase.test.ts already uses -- no other instance found. Reproduced the actual CI condition to confirm the fix, not just a plausible one: started a real postgres:16 container on host port 5432, confirmed the pre-fix isPostgresReachable logic returns true against it (reproducing the exact failure), then ran the new test file in the same environment -- 8/8 green, since nothing in it probes 5432 anymore. Also re-proved ordinary red-then-green on the new resolveHostPort assertions (temporarily defaulted to 5433, saw the one expected failure, reverted, saw 8/8 again). Stopped the test container afterward.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/db/postgresReachable.ts`:
- Around line 51-54: Update resolveHostPort so an explicitly provided port of
"0" is preserved, using the default 5432 only when url.port is empty. Add a
regression test covering postgresql://127.0.0.1:0/whatever and assert that the
resolved port is 0.
In `@CHANGES.md`:
- Around line 58-61: Update the documented “Guaranteed-closed” claim in
CHANGES.md to remove the assertion that 127.0.0.1:1 is guaranteed unreachable,
and describe the tests as using a deterministic connection-failure seam instead
of an unused local port.
🪄 Autofix
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: dcd0d1e1-0c41-4ea8-9b8d-4a03f006831b
📒 Files selected for processing (3)
CHANGES.mdapi/src/db/__tests__/postgresReachable.test.tsapi/src/db/postgresReachable.ts
Comment on lines
+58
to
+61
| - **Guaranteed-closed, not "probably closed":** the "resolves false"/"exits non-zero" cases both use | ||
| `127.0.0.1:1` — port 1 is a reserved well-known port nothing binds to as a listener; this is the | ||
| pre-existing pattern `ensureDatabase.test.ts` already uses for its identical "unreachable" case, | ||
| not something new introduced here. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the guaranteed-closed claim for port 1.
127.0.0.1:1 is not guaranteed to refuse connections. A privileged local process can listen on port 1, so the test can still depend on host state. Use a deterministic connection-failure seam instead of relying on an unused port.
🤖 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 `@CHANGES.md` around lines 58 - 61, Update the documented “Guaranteed-closed”
claim in CHANGES.md to remove the assertion that 127.0.0.1:1 is guaranteed
unreachable, and describe the tests as using a deterministic connection-failure
seam instead of an unused local port.
… by identity, run from anywhere
CodeRabbit findings on audit/requirements/pipeline/acceptance-check.py:
- Line 9 (Major): `active` was built from every `## W4-R` heading, retired
ones included, and the retired *count* came from a literal substring match
(`inv.count("Status: retired")`) that never matches this repo's actual
Markdown-bolded status line (`- **Status:** retired`) — so it was always 0.
The fallback then accepted any missing rows whenever their count happened
to equal the retired count, which is a match on quantity, not identity: N
requirements missing and N requirements retired does not mean they're the
same N. Rewrote to parse each requirement's own section (mirroring
merge-matrix.py's already-correct `active_ids()`), exclude only IDs whose
own section states retired, and fail on every missing active ID with the
IDs named.
- Line 16 (Major): every check was a bare `assert`, which `python -O`
compiles out entirely — a gate whose checks vanish under an interpreter
flag is not a gate. Replaced with explicit conditionals that call a
`fail()` helper (`sys.exit(1)` after printing to stderr).
- Line 4 (Major) + Ruff SIM115: paths were relative, requiring cwd == repo
root, contradicting pipeline/README.md ("run from anywhere") and the
sibling scripts, which already derive the repo root from `__file__`.
Switched to `Path(__file__).resolve().parents[3]` (same derivation
merge-matrix.py uses) and `with open(...) as f:` context managers.
Also fixes Ruff E741 (ambiguous variable name `l`).
Verified against copies in a scratch dir (real audit artifacts untouched):
constructed a case with one requirement retired (plain-format status, which
the old script's naive check *does* recognize) and a different, genuinely
active requirement dropped from the matrix — count-equal (1 retired, 1
missing) but wrong identity. Old script: `OK — 53 rows, verdicts sound`,
exit 0 — silently wrong. New script:
`FAIL: 1 active requirement(s) missing from the matrix: W4-R1`, exit 1.
Also confirmed under `python -O` (still fails — no assert to compile out)
and from `/tmp` as cwd (still finds its inputs; the old script raises
FileNotFoundError there). Re-run against the actual, unmodified
audit/requirements/ tree: `OK — 54 rows, verdicts sound`, exit 0, from both
the repo root and an unrelated cwd.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…error before reading .code
Two CodeRabbit findings I judged (bodies were truncated in the fetch, read
in full via the GitHub API):
- postgresReachable.ts:54 (Minor, correctness): `resolveHostPort` computed
`Number(url.port) || 5432`. `Number('0')` is `0`, which is falsy, so an
explicit `postgresql://host:0/db` silently resolved to port 5432 instead
of the port the URL actually specified. Real bug — confirmed `url.port`
is genuinely `'0'` (not `''`) for that input. Changed to
`url.port === '' ? 5432 : Number(url.port)`, which only defaults on a
truly absent port, and added a regression test asserting
`resolveHostPort('postgresql://127.0.0.1:0/whatever')` resolves to
`{ host: '127.0.0.1', port: 0 }`.
- postgresReachable.test.ts:63 (Minor, maintainability): `runCli`'s catch
block read `(error as { code?: unknown }).code` — an unchecked assertion
on a catch variable typed `unknown`. If `error` were ever null or a
primitive, that read throws its own TypeError and masks the original
failure instead of rethrowing it. Low-probability for `execFile`'s actual
rejection shape, but the check is genuinely free: added
`isErrorWithNumericCode`, a real type guard (`typeof error === 'object' &&
error !== null && 'code' in error && typeof ...code === 'number'`) in
place of the cast, matching this repo's stated preference for typing
`unknown` boundaries explicitly instead of asserting through them.
`npx vitest run postgresReachable.test.ts`: 9/9 passing (was 8, +1 for the
new port-0 case). `pnpm --filter @ship/api type-check`: clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ments snapshot as frozen Three more CodeRabbit findings from PR #154, none touching application code: - CHANGES.md:61: "port 1 is a reserved well-known port nothing binds to as a listener" was stated as a guarantee. It isn't — a privileged (root or admin-equivalent) process can legally listen on port 1, so the test's "unreachable" cases still depend on host state, just far less likely to matter than the port-5432 case this same entry documents fixing. Reworded to "conventionally unused, not guaranteed-closed" and named the residual risk explicitly, per this repo's own rule against stating an environment assumption as a guarantee. - docs/superpowers/specs/2026-08-08-requirements-audit-skill-design.md:83: the config schema example nested the app URL as `verify.app_url`. Both the real `audit/requirements.config.yaml` and the implementation plan (`docs/superpowers/plans/2026-08-08-requirements-audit-skill.md:32`) use a separate top-level `verify_urls.app`. A skill built from this spec would look up a field that doesn't exist. Split the example into `verify:` + `verify_urls:` to match. - REPORT.md / gaps.md / pm-triage.md (the stale-audit-artifact family, 4 findings total): CodeRabbit is right that these now contradict docs/IMPROVEMENTS.md's corrected type-safety verdict and the fixed migration test — both landed on this branch (8386f26, 5126a03) after these artifacts were last generated at commit ef87839, along with the W4-R42 Postgres-bootstrap fix (5030ad0). NOT regenerating: these three files plus matrix.baseline.json are the frozen "before" state W4-R34's before/after proof depends on, and the cluster-*.json / verification- results.json trace evidence (also flagged, left untouched) is recorded evidence of what a sweep agent actually saw — rewriting it to agree with a later world defeats the purpose of keeping it. Instead added a short header to each of the three human-facing docs stating plainly: this describes commit ef87839, is a frozen baseline, W4-R35/W4-R10(a)/W4-R42 fixes have since landed, and a compare-mode run — not an edit to this file — is what will show the delta. `node scripts/factory/merge-changes.mjs --check CHANGES.md`: OK, 144 entries, 230 fences (pre-existing warnings only, unrelated to this diff). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CHANGES.md (1)
42-45: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the
CHANGES.mdport rule.resolveHostPortdefaults to5432only when no port is specified. An explicit:0resolves to port0. ReplaceNumber(url.port) || 5432and qualify “behavior unchanged” to include the port-0 fix.🤖 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 `@CHANGES.md` around lines 42 - 45, Update the CHANGES.md entry for resolveHostPort to document that port 5432 is used only when no port is specified, while an explicit :0 resolves to port 0; replace the Number(url.port) || 5432 wording and qualify “behavior unchanged” to note this port-0 correction.audit/requirements/gaps.md (1)
61-68: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse one dependency-count definition for W4-R38.
audit/requirements/gaps.mdcorrectly reports 142 caret ranges, 9 exact pins, and 2workspace:*references. Updateaudit/requirements/pm-triage.md, which incorrectly describes all 144 non-exact entries as caret ranges. Treat the 2 internal workspace references separately and limit pinning changes to the 142 caret ranges unless repository policy requires otherwise.🤖 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 `@audit/requirements/gaps.md` around lines 61 - 68, The dependency count for W4-R38 must distinguish 142 caret ranges from 2 internal workspace:* references: update audit/requirements/pm-triage.md lines 105-116 to describe the two categories separately and limit pinning work to the 142 caret entries unless repository policy requires otherwise; audit/requirements/gaps.md lines 61-68 is the anchor and requires no direct change because it already reports the correct breakdown.
🤖 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 `@api/src/db/postgresReachable.ts`:
- Line 59: Update the host construction in the PostgreSQL URL parsing logic to
remove outer brackets from IPv6 literals before returning the host, while
preserving ordinary hostnames and the localhost fallback. Add a regression test
covering postgresql://[::1]:5432/whatever and assert that the returned host is
::1.
In `@audit/requirements/pipeline/acceptance-check.py`:
- Around line 90-92: Update the gap-loading and validation logic around the
MISSING verdict check to parse Markdown gap headings into a set of exact
requirement IDs, then test membership against that set instead of
substring-searching the full file. Ensure W4-R1 does not match W4-R10 or
unrelated prose, and add a regression case covering both prefix IDs.
- Around line 86-95: Update the matrix validation loop in the acceptance-check
logic to reject verdicts outside VERIFIED, MISSING, and ASSUMED before returning
OK. For VERIFIED result_excerpt and ASSUMED assumption, require trimmed,
non-empty string values rather than merely truthy values, while preserving the
existing gaps check for MISSING. Add regression cases covering an unknown
verdict and whitespace-only evidence or assumption.
---
Outside diff comments:
In `@audit/requirements/gaps.md`:
- Around line 61-68: The dependency count for W4-R38 must distinguish 142 caret
ranges from 2 internal workspace:* references: update
audit/requirements/pm-triage.md lines 105-116 to describe the two categories
separately and limit pinning work to the 142 caret entries unless repository
policy requires otherwise; audit/requirements/gaps.md lines 61-68 is the anchor
and requires no direct change because it already reports the correct breakdown.
In `@CHANGES.md`:
- Around line 42-45: Update the CHANGES.md entry for resolveHostPort to document
that port 5432 is used only when no port is specified, while an explicit :0
resolves to port 0; replace the Number(url.port) || 5432 wording and qualify
“behavior unchanged” to note this port-0 correction.
🪄 Autofix
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: dba02a91-e684-4170-aed2-865f194912b3
📒 Files selected for processing (8)
CHANGES.mdapi/src/db/__tests__/postgresReachable.test.tsapi/src/db/postgresReachable.tsaudit/requirements/REPORT.mdaudit/requirements/gaps.mdaudit/requirements/pipeline/acceptance-check.pyaudit/requirements/pm-triage.mddocs/superpowers/specs/2026-08-08-requirements-audit-skill-design.md
…ost to net.createConnection
CodeRabbit re-review, postgresReachable.ts:59 (Minor, functional correctness).
I fetched and read the full analysis chain, then reproduced the failure
directly rather than reasoning about it:
node -e "console.log(new URL('postgresql://[::1]:5432/x').hostname)"
// -> '[::1]' (brackets included)
net.isIP('[::1]') // -> 0 (not recognized as an IP at all)
net.isIP('::1') // -> 6 (correctly IPv6)
dns.lookup('[::1]', ...) // ENOTFOUND
dns.lookup('::1', ...) // resolves
Then reproduced it end-to-end against a real listener: a `net.Server` bound
to `::1`, connected to with `{ host: '[::1]', port }` (what `resolveHostPort`
returned before this fix) fails with `ENOTFOUND`; the identical connection
with `{ host: '::1', port }` succeeds. So every IPv6 `DATABASE_URL` was
previously read as unreachable regardless of whether Postgres was actually
listening — a real bug, not a hypothetical one.
**What changed.** `resolveHostPort` now strips a leading `[` and trailing
`]` from `url.hostname` before returning it, only when both are present
(ordinary hostnames and the empty-hostname → 'localhost' fallback are
unaffected — neither starts with `[`).
**Tests added**, matching the file's stated no-mocked-sockets philosophy:
- `resolveHostPort('postgresql://[::1]:5432/whatever')` → `{ host: '::1', port: 5432 }`
- `resolveHostPort('postgresql://[2001:db8::1]:5432/whatever')` → unbracketed host
- End-to-end: `isPostgresReachable` resolves `true` against a real listener
bound to `::1`, addressed via the bracketed URL form Postgres connection
strings actually use — this is the case that was silently broken before
the fix (always resolved `false`).
`npx vitest run postgresReachable.test.ts`: 12/12 passing (was 9; +3 new
IPv6 cases, including the loopback-listener bind, which this sandbox
supports). `pnpm --filter @ship/api type-check`: clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h, unknown-verdict passthrough Second round of CodeRabbit review on the same gate this branch already hardened once (575c2fa). Two real findings, same false-pass class: 1. **Line 92 (Major): MISSING rows matched by substring.** `r["id"] not in gaps` searched gaps.md's raw text, so `"W4-R1" in gaps_text` was satisfied by a `### W4-R10` heading (or any prose mention) — a genuinely MISSING W4-R1 with no write-up of its own would pass. Added `parse_gap_ids()`, anchored to `^### (W4-R\d+)(?:\s|$)` (MULTILINE), and check membership against that set instead. Demonstrated in a scratch copy (real audit/requirements/ untouched, confirmed via `git status`): W4-R1 set to MISSING in the matrix, gaps.md left with only its pre-existing `### W4-R10` heading and no `### W4-R1` heading. - Old script (575c2fa): `OK — 54 rows, verdicts sound`, exit 0. - New script: `FAIL: W4-R1: MISSING but absent from gaps.md`, exit 1. 2. **Line 95 (Major): unrecognized verdicts skip every branch.** The loop only branched on `VERIFIED`/`MISSING`/`ASSUMED`; a typo (`VERIFED`) or any out-of-vocabulary value matched none of them and fell straight through to `OK`. Added `VALID_VERDICTS` — the exact 7-value table from `report-format.md` — and fail immediately on anything outside it, naming the row and the bad value. Also tightened the truthiness checks: `result_excerpt`/`assumption` accepted a whitespace-only string as "evidence" under plain `if value:`; added `has_content()` (`isinstance(str) and value.strip() != ""`). Demonstrated the same way: W4-R13's verdict corrupted to `'VERIFED'` with whitespace-only evidence. - Old script: `OK — 54 rows, verdicts sound`, exit 0. - New script: `FAIL: W4-R13: unrecognized verdict 'VERIFED'`, exit 1. Isolated the whitespace-truthiness half separately (verdict left as the valid `VERIFIED`, evidence still whitespace-only): old script still prints `OK` (exit 0); new script: `FAIL: W4-R13: VERIFIED without evidence` (exit 1). **A fourth issue, found on the requested full re-read, not from CodeRabbit:** report-format.md's field rules state `suggested_scope` is "non-null for every MISSING and PARTIAL row ... an unset value silently drops a required field from the PM handoff" — the acceptance gate never checked this at all. Added the same class of check: any MISSING/PARTIAL row with a blank `suggested_scope` now fails, naming the row. Verified the real matrix's 10 PARTIAL rows all already carry non-empty `suggested_scope` (this addition changes nothing about the current `OK`). Considered and deliberately NOT added: verifying `verification`/`assumption` are null when the verdict doesn't require them, and that BLOCKED rows carry a non-blank `notes`. Both are already enforced at write time in `merge-matrix.py` (`if verdict != 'VERIFIED': verification = None`, etc.), so a reader-side check would be pure defense-in-depth rather than closing a live false-pass path, and report-format.md doesn't use the same "silently drops a required field" language for them that motivated the suggested_scope check. No BLOCKED rows exist in the current 54-row matrix either way. Refactored the checks into a pure `find_first_violation(matrix, active_ids, gap_ids)` (no file I/O) so they're testable directly. Added `test_acceptance_check.py` (stdlib `unittest`, loaded via `importlib` since the module under test has a hyphenated filename): 14 cases, including the exact W4-R1/W4-R10 prefix collision and the corrupted-verdict/whitespace- evidence cases above. `python3 audit/requirements/pipeline/ test_acceptance_check.py`: 14/14 passing. Re-ran the real, unmodified pipeline: `OK — 54 rows, verdicts sound`, exit 0, from both the repo root and an unrelated cwd. Updated pipeline/README.md's file table (the "verbatim" claim about the plan doc's snippet is now stale — this script has diverged from it on purpose) and added the new test file's row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/src/db/__tests__/postgresReachable.test.ts`:
- Around line 127-137: Replace the IPv6-dependent server setup in the postgres
reachability test with a mock of net.createConnection that emits connect, and
assert the connection options specify host: '::1'. Remove the server.listen/port
allocation path while preserving the existing real IPv4 reachability test as the
TCP integration coverage.
In `@audit/requirements/pipeline/acceptance-check.py`:
- Around line 127-134: Update the requirement validation around
matrix["requirements"] and matrix_ids to detect duplicate IDs before converting
or relying on the set, and reject any IDs not present in active_ids. Return a
validation error for either condition before the existing missing_active check,
and add regression tests covering duplicate and unknown requirement rows.
🪄 Autofix
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: c0efa7cc-0fc4-4d52-acff-f590ff034fb6
📒 Files selected for processing (5)
api/src/db/__tests__/postgresReachable.test.tsapi/src/db/postgresReachable.tsaudit/requirements/pipeline/README.mdaudit/requirements/pipeline/acceptance-check.pyaudit/requirements/pipeline/test_acceptance_check.py
Comment on lines
+127
to
+137
| server = net.createServer(); | ||
| const port = await new Promise<number>((resolve, reject) => { | ||
| server?.listen(0, '::1', () => { | ||
| const address = server?.address(); | ||
| if (address && typeof address === 'object') { | ||
| resolve(address.port); | ||
| } else { | ||
| reject(new Error('server did not report a port')); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'NODE'
const net = require('node:net');
const server = net.createServer();
server.once('error', error => {
console.error(`IPv6 loopback is unavailable: ${error.code ?? error.message}`);
process.exitCode = 1;
});
server.listen(0, '::1', () => {
console.log('IPv6 loopback is available');
server.close();
});
NODERepository: troysatchell/ship
Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="api/src/db/__tests__/postgresReachable.test.ts"
printf '%s\n' '--- test structure ---'
sed -n '1,190p' "$file"
printf '%s\n' '--- relevant symbols and scripts ---'
rg -n -C 3 "createServer|listen\\(|createConnection|postgresReachable|IPv6|::1|describe\\(|it\\(|test\\(" \
"$file" package.json api/package.json 2>/dev/null || true
printf '%s\n' '--- deterministic IPv6 listener probe ---'
node <<'NODE'
const net = require('node:net');
const server = net.createServer();
let callbackCalled = false;
let errorCode = null;
server.once('error', error => {
errorCode = error.code ?? error.message;
console.log(JSON.stringify({
ipv6LoopbackAvailable: false,
error: errorCode,
callbackCalled,
}));
});
server.listen(0, '::1', () => {
callbackCalled = true;
const address = server.address();
console.log(JSON.stringify({
ipv6LoopbackAvailable: true,
address,
callbackCalled,
hasErrorListener: server.listenerCount('error') > 0,
}));
server.close();
});
NODERepository: troysatchell/ship
Length of output: 23022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- reachability implementation ---'
fd -i 'postgresReachable' .
for file in $(fd -i 'postgresReachable' . | head -20); do
if [ -f "$file" ]; then
printf '\n--- %s ---\n' "$file"
wc -l "$file"
sed -n '1,220p' "$file"
fi
done
printf '%s\n' '--- existing net mocking patterns ---'
rg -n -C 4 "vi\.(mock|spyOn)|mock\(.*node:net|createConnection|listen\\(" \
--glob '*.{ts,tsx}' api/src | head -300
printf '%s\n' '--- failure behavior without an error listener ---'
node <<'NODE'
const net = require('node:net');
const server = net.createServer();
process.once('uncaughtException', error => {
console.log(JSON.stringify({
uncaughtException: true,
code: error.code ?? null,
message: error.message,
}));
process.exitCode = 0;
});
server.listen(0, '2001:db8::1', () => {
console.log(JSON.stringify({ unexpectedSuccess: true }));
server.close();
});
NODERepository: troysatchell/ship
Length of output: 41396
Remove the IPv6 host dependency from this test.
When the CI executor has IPv6 disabled, server.listen(0, '::1') emits EADDRNOTAVAIL. The promise has no error listener, so the test can fail with an uncaught exception.
Mock net.createConnection, emit connect, and assert that the options use host: '::1'. Keep the real IPv4 reachability test as the TCP integration test.
🤖 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 `@api/src/db/__tests__/postgresReachable.test.ts` around lines 127 - 137,
Replace the IPv6-dependent server setup in the postgres reachability test with a
mock of net.createConnection that emits connect, and assert the connection
options specify host: '::1'. Remove the server.listen/port allocation path while
preserving the existing real IPv4 reachability test as the TCP integration
coverage.
…nown IDs pass
Third round of CodeRabbit review on this gate. Both findings real, same
false-pass class as rounds 1 and 2:
**Line 134 (Major): duplicates and unknown IDs both pass.**
`matrix_ids = {r["id"] for r in matrix["requirements"]}` silently collapses
a repeated ID to one set member, and the missing-row check only ever asks
"is every active ID present in matrix_ids" — never the other direction, "is
every matrix ID actually active". So a matrix containing every active ID
PLUS a duplicate row, or PLUS an invented ID, reached every check unchanged
and printed OK.
Added two checks before the existing missing-row check: list-length vs
set-length catches duplicates (names the repeated IDs); `matrix_ids -
set(active_ids)` catches unknown IDs (names them). Confirmed against
merge-matrix.py's own reqs loop (`for rid in ids:`, where `ids` is the
active-only list) that a legitimate matrix never contains a row outside
active_ids — so this isn't a new constraint, it's enforcing one the pipeline
already assumes but never checked on the read side.
Demonstrated in a scratch copy (real audit/requirements/ confirmed
untouched via `git status`), against the previous commit's version (def242f):
Duplicate-row scenario (every active ID present + a second W4-R1 row):
Old: `OK — 55 rows, verdicts sound`, exit 0.
New: `FAIL: 1 requirement ID(s) appear more than once in the matrix: W4-R1`, exit 1.
Unknown-ID scenario (every active ID present + an invented W4-R99 row):
Old: `OK — 55 rows, verdicts sound`, exit 0.
New: `FAIL: 1 requirement ID(s) in the matrix are not active (unknown or
should-be-excluded-retired): W4-R99`, exit 1.
**The inverted-question pass, as asked.** Enumerated the matrices that
SHOULD fail the gate — missing row, duplicate row, unknown row, bad verdict,
empty evidence, VERIFIED without verification, MISSING absent from gaps,
ASSUMED without assumption, PARTIAL without suggested_scope (9 categories,
9b added: MISSING without suggested_scope, since the same check covers both
verdicts but only PARTIAL had a case) — and checked test_acceptance_check.py
against it. Full result in FindFirstViolationTests' new class docstring:
Already existed: missing row, bad verdict, empty evidence (whitespace
variant), MISSING-absent-from-gaps, ASSUMED-without-assumption (whitespace
variant), PARTIAL-without-suggested_scope. 6 of 9(b).
Added this round: duplicate row, unknown row, VERIFIED-without-
verification (verification key absent, distinct from present-but-blank),
ASSUMED-without-assumption (assumption key absent, same distinction),
MISSING-without-suggested_scope. 5 new cases.
`python3 audit/requirements/pipeline/test_acceptance_check.py`: 19/19
passing (was 14, +5). Re-ran the real, unmodified pipeline: `OK — 54 rows,
verdicts sound`, exit 0, from repo root and an unrelated cwd.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…obe, not an assumption CodeRabbit, postgresReachable.test.ts:137 (Major). The "resolves true against an IPv6 listener" test unconditionally called `server.listen(0, '::1', ...)`. On a host with IPv6 loopback disabled that emits `EADDRNOTAVAIL`/`EAFNOSUPPORT` on the server's 'error' event, which this Promise-wrapped listen had no listener for — an uncaught exception, and one that looks exactly like a code defect rather than what it actually is: an environment assumption. Third instance of this class on this branch, after the port-1 "guaranteed closed" claim and the port-5432 default-port test, both already documented in CHANGES.md. **The fix that was explicitly wrong, and why:** blanket-skipping or mocking-away the IPv6 case entirely would make this the same defect as the 68 e2e specs that pass without asserting anything — a green result that means nothing. Also wrong: gating on `os.networkInterfaces()` or a similar heuristic, since the test is about to attempt a bind and the heuristic could disagree with the real bind's own failure mode. **What changed.** Added `ipv6LoopbackAvailable()`: attempts the real bind, resolves `true`/`false` for the two specific error codes that mean "not available here" (`EADDRNOTAVAIL`, `EAFNOSUPPORT`), and rethrows anything else — an unexpected bind failure should fail loudly, not be silently absorbed into "must be environmental". Decided once via top-level await (this file is ESM) before `describe`/`it` register, so every test in the file sees the same answer. The IPv6 end-to-end test is now registered with `(IPV6_LOOPBACK_AVAILABLE ? it : it.skip)(...)` — a real Vitest skip, which shows as `↓ ... 1 skipped` in the report, not silently folded into "passed". A `console.warn` fires unconditionally when it skips, naming exactly which case and why. The pure `resolveHostPort` IPv6 bracket-stripping assertions (added in 2531ca1) are untouched: no socket, no environment dependency, still run unconditionally — those are the actual guarantee against the bracket-stripping regression; the end-to-end case is corroborating evidence on top, not the only place it could be caught. **Verified both branches, not just reasoned about them.** This sandbox has IPv6 loopback available (confirmed directly: `node -e "..."` binds `::1` successfully), so the normal run exercises the real `it` path: 12/12 passing, all named tests including the IPv6 one visibly ran (`--reporter= verbose`). Then temporarily forced `IPV6_LOOPBACK_AVAILABLE = false` to prove the *other* branch actually works — confirmed the `console.warn` printed, the test showed as `↓ ... skipped` (11 passed | 1 skipped, not 12 passed), and the other 11 cases including all 5 pure `resolveHostPort` IPv6/non-IPv6 cases still ran and passed — then reverted (`diff` against a backup confirmed a byte-for-byte clean revert before committing). `npx vitest run postgresReachable.test.ts`: 12/12 passing. `pnpm --filter @ship/api type-check`: clean. `npx eslint api/src/db/__tests__/postgresReachable.test.ts`: clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three items from the W4 requirements sweep's PM scope gate (
audit/requirements/pm-triage.md), bundled into one branch and one review. None has a Linear ticket — deliberate PM ruling: they were dispositioned "fix now", and filing tickets for work being done in the same session is overhead rather than coordination.Gate:
pass— all checks green, includingtests:not-weakenedandregression-test(4 cases in 2 files). Evidence in.factory/gate-result.json.1. W4-R35 — the suite is red, for one trivial reason (
5126a03)Problem.
pnpm testexits 1. Two failures, both inapi/src/db/__tests__/migrationRunner.test.ts(lines 167, 184), which is the regression test for DB-1 / TRO-178.Cause — observed, not inferred. The test compared a Postgres-ordered query (
SELECT version FROM schema_migrations ORDER BY version) against a JavaScript[...expected].sort(). Those collations disagree on exactly one pair out of 46 migration names. Verified directly:Nothing was wrong with the migrations. The collision was introduced by
426f0e2("fix: resolve migration numbering collisions") — the fix for numbering collisions created an ordering one. It is the only such pair among all 46.Change.
recordedVersions()now sorts in JS after the query, so both sides of every comparison use one collation. The SQLORDER BYstays for anyone reading the query in isolation.What was deliberately NOT done. The assertion remains a strict, ordered
toEqualon full identity. It was not loosened to a count, a subset check, orexpect.arrayContaining— any of which would have gone green by destroying the DB-1 guarantee the test exists to enforce ("the runner must apply every migration or fail"). A runner that skips or duplicates a migration still fails this test.Why this one first. It repairs the second clause of this project's own definition of done — "the full suite still passes" — which currently fails for all 121 tickets marked Done, and it was blocking the
tests:apigate on every other branch.2. W4-R10(a) — a stated target reported as met (
8386f26)Problem.
docs/IMPROVEMENTS.mdrecorded the type-safety category's verdict as met. The requirement's threshold is defined on thecount.shtracked total, and that total has never measured below baseline.Measurement — re-run this session with the exact command
audit/type-safety/baseline.md:14-18prescribes:+452 / +29%, where W4-R10 requires −25% (target ≤1151). Breakdown: any 50 + as 1882 + non-null 47 + ts-ignore 8, summed by
baseline.md:77's own formula.Precision about the defect. This was not a hidden claim. The document already marked its inference explicitly and printed "1535 → 1778, Up 243" in the open. The narrow, defensible correction is that the per-ticket sum-of-diffs answers a different question than the threshold names. That argument is preserved in the document, demoted from verdict-justification to supporting evidence — the real wins are real (
anyhalved 102 → 50;req.userId!/req.workspaceId!236 → 0) and are still recorded.The file grew (+62/−35): nothing honest was removed.
3. W4-R42 — one-command start stopped at the database (
5030ad0)Problem. W4-R42 requires one command to start the full composed system "without any manual setup steps beyond installing dependencies."
./start.shdid everything correctly given a reachable Postgres, but did not start Postgres itself — it failed atapi/src/db/ensureDatabase.ts:53-61.Observed, first-hand: this is what blocked the requirements sweep earlier today on a clean machine —
ERROR: Cannot reach PostgreSQL at localhost:5433. Start it, then re-run ./start.sh.Change.
scripts/dev.shnow brings thedocker-compose.local.ymlPostgres up when unreachable and waits for health, falling back to the original message when Docker is unavailable. Newapi/src/db/postgresReachable.tswith its own test.ensureDatabase.tsis unchanged — its message is correct and is cited as evidence elsewhere.Bundled rather than split because CodeRabbit rate-limited this factory once on PR-per-ticket. Dependency pinning (W4-R38) and the Terraform plan annotations (W4-R27) are deliberately separate PRs: each would have swamped review of these three.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation