Pay down static text witness batch A#657
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a dead-export CLI (scan, TS parse, export extraction, reference counting, Markdown report) with fixtures/tests; introduces MarkdownDocument test helper; refactors tests to use structured markdown queries and TypeScript AST checks; adds Zod manifest validation and removes one source-pattern conformance test. ChangesDead Export Detection Tool
Test Infrastructure Improvements
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.ts (1)
26-38: ⚡ Quick winMake
ByteCollectableStream.collecthonor theCollectableStreamoptions contract.The test double currently ignores optional
collectoptions, which can make stream-path tests less representative of the adapter contract.Suggested patch
class ByteCollectableStream implements CollectableStream { constructor(private readonly chunks: readonly Uint8Array[]) {} async *[Symbol.asyncIterator](): AsyncIterator<Uint8Array> { for (const chunk of this.chunks) { yield chunk; } } - async collect(): Promise<Buffer | string> { - return Buffer.concat(this.chunks.map((chunk) => Buffer.from(chunk))); + async collect(opts?: { asString?: boolean; maxBytes?: number }): Promise<Buffer | string> { + const buffer = Buffer.concat(this.chunks.map((chunk) => Buffer.from(chunk))); + if (typeof opts?.maxBytes === 'number' && buffer.byteLength > opts.maxBytes) { + throw new Error('stream exceeds maxBytes'); + } + return opts?.asString ? buffer.toString('utf8') : buffer; } }🤖 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 `@test/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.ts` around lines 26 - 38, The test stub ByteCollectableStream currently implements collect() but ignores the CollectableStream options contract; update ByteCollectableStream.collect to accept the optional options parameter (matching the CollectableStream signature) and return a string when an encoding option is provided or a Buffer otherwise. Specifically, change the collect signature on ByteCollectableStream to accept options (e.g., collect(options?: { encoding?: string })), use Buffer.concat(this.chunks.map(...)) to build the result, and if options?.encoding is set return result.toString(options.encoding) else return the Buffer; ensure the async iterator and other behavior remain unchanged so tests that pass collect options exercise the same contract as the real adapter.
🤖 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 `@test/unit/domain/trust/domainPurity.test.ts`:
- Around line 36-43: moduleSpecifiers currently only extracts module specifiers
from ImportDeclaration nodes, so export statements like `export ... from
'.../infrastructure/'` are missed; update moduleSpecifiers to also handle
ExportDeclaration by checking ts.isExportDeclaration(statement) and
ts.isStringLiteral(statement.moduleSpecifier) and include
statement.moduleSpecifier.text in the returned array (preserve the existing
ImportDeclaration handling in the same function so both import and export-from
specifiers are collected for the boundary checks).
---
Nitpick comments:
In `@test/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.ts`:
- Around line 26-38: The test stub ByteCollectableStream currently implements
collect() but ignores the CollectableStream options contract; update
ByteCollectableStream.collect to accept the optional options parameter (matching
the CollectableStream signature) and return a string when an encoding option is
provided or a Buffer otherwise. Specifically, change the collect signature on
ByteCollectableStream to accept options (e.g., collect(options?: { encoding?:
string })), use Buffer.concat(this.chunks.map(...)) to build the result, and if
options?.encoding is set return result.toString(options.encoding) else return
the Buffer; ensure the async iterator and other behavior remain unchanged so
tests that pass collect options exercise the same contract as the real adapter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b8860d5-e471-4b58-b912-169e85c88203
📒 Files selected for processing (18)
package.jsonscripts/dead-export-report.tstest/conformance/hygieneQuarantineGraduation.test.tstest/conformance/immutableSnapshotBuilder.test.tstest/fixtures/dead-export-report/src/consumer.tstest/fixtures/dead-export-report/src/index.tstest/fixtures/dead-export-report/src/renamed-unused-function.tstest/fixtures/dead-export-report/src/unused-value.tstest/fixtures/dead-export-report/src/used-value.tstest/helpers/MarkdownDocument.tstest/unit/domain/trust/domainPurity.test.tstest/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.tstest/unit/scripts/dead-export-report.test.tstest/unit/scripts/documentation-corpus-shape.test.tstest/unit/scripts/factory-functions-in-tests-shape.test.tstest/unit/scripts/glossary-shape.test.tstest/unit/scripts/incremental-index-updater-closeout-shape.test.tstest/unit/scripts/index-builder-on-git-cas-shape.test.ts
💤 Files with no reviewable changes (1)
- test/conformance/immutableSnapshotBuilder.test.ts
Member
Author
|
@codex self-review found issues that should be resolved before merge. Evidence below is deterministic and reproducible against
Current checks observed during review:
|
Member
Author
|
@codex review-fix update pushed in
Local validation completed before push:
|
Release Preflight
If you tag this commit as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Batch A for the next static-text witness burndown.
This PR replaces brittle source/prose assertions with deterministic witnesses:
npm run issue:dead-exportsbacked byscripts/dead-export-report.tsfor Static text assertions intest/unit/scripts/dead-code-cleanup-shape.test.ts#296.test/helpers/MarkdownDocument.tsfor structured heading/link/table/task-row checks.Issues
Refs #296, #298, #299, #300, #301, #302, #304, #305, #306, #307.
No issue is auto-closed by this PR body. After merge, each issue should be closed only with concrete evidence anchored to merged paths, line numbers, and commit SHA.
Evidence
npm exec vitest run test/unit/scripts/dead-export-report.test.ts test/unit/scripts/documentation-corpus-shape.test.ts test/unit/domain/trust/domainPurity.test.ts test/unit/scripts/factory-functions-in-tests-shape.test.ts test/unit/infrastructure/adapters/GitGraphAdapter.gitCasPersistence.test.ts test/unit/scripts/glossary-shape.test.ts test/conformance/hygieneQuarantineGraduation.test.ts test/conformance/immutableSnapshotBuilder.test.ts test/unit/scripts/incremental-index-updater-closeout-shape.test.ts test/unit/scripts/index-builder-on-git-cas-shape.test.tsnpm run issue:dead-exports -- test/fixtures/dead-export-report/srcnpm run issue:dead-exports -- srcnpm run lintnpm run typechecknpm run lint:mdnpm run lint:linksnpm run test:localNotes
The dead-export scanner is deliberately a candidate report, not a release-blocking ratchet. It gives deterministic reachability evidence for #296 without pretending every zero-reference export is safe to delete.
Summary by CodeRabbit
New Features
Tests