feat: import foundation layer (compression primitives, envelope, state store) - #1
Merged
Merged
Conversation
…e store basics) Import from private predecessor, first of a series of dependency-ordered PRs. Includes deterministic compression primitives (ANSI/JSON/lines/code skeleton/tool-description/budget), the result envelope contract, request logging, telemetry sink, TTL-bounded artifact retention, read-governor classification/evidence groundwork, OAuth broker helper, and the cross-platform state directory resolver. Renames the project-scoped state directory and log prefixes from the predecessor's mottainai-nosy-mcp to mottainai. Proxy relay, upstream connections, tool catalog, adaptive routing, and the CLI entry point land in follow-up PRs as their dependencies are satisfied. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMottainai gateway foundation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (4)
src/adaptive/trace.test.ts (1)
1-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for file rotation and TTL sweep.
The test suite does not exercise
MOTTAINAI_TRACE_MAX_FILE_BYTESrotation or theMOTTAINAI_TRACE_RETENTION_DAYSstartup sweep in trace.ts. Both are documented, non-trivial persistence behaviors that other layers will depend on. Add a test that sets a smallMOTTAINAI_TRACE_MAX_FILE_BYTESand verifies multiple files get created, and a test that seeds an aged file (viafs.utimesSync) and verifies it is removed on the next store creation.🤖 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 `@src/adaptive/trace.test.ts` around lines 1 - 174, Extend the trace store tests with coverage for persistence rotation and retention: add a test using MOTTAINAI_TRACE_MAX_FILE_BYTES with a small limit that records enough data through createTraceStore to produce multiple JSONL files, and add a test that ages a trace file with fs.utimesSync, sets MOTTAINAI_TRACE_RETENTION_DAYS, then creates a new store and verifies the aged file is removed. Anchor both tests near the existing createTraceStore persistence tests and preserve current temporary-directory setup.src/telemetry.ts (2)
166-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
snapshot()exposes internal mutable state by reference.
snapshot()spreadsstateshallowly, sototals,by_provider, andby_capabilityin the returned object are the same references held internally (Line 44-48). A caller that mutates a returned snapshot would silently corrupt the sink's ongoing aggregation.Deep-clone the returned counters (e.g. via
structuredCloneor a manual copy) to makesnapshot()safe against caller mutation.🤖 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 `@src/telemetry.ts` around lines 166 - 169, Update the snapshot() method to deep-clone the mutable totals, by_provider, and by_capability counters before returning them, ensuring callers cannot mutate the sink’s internal state while preserving the existing snapshot metadata.
135-165: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider batching telemetry persistence instead of writing on every call.
persist()is invoked on everyrecordToolCallandrecordRetrieval(Line 160, Line 164), each time serializing the entire aggregate state and writing it to disk (Lines 138-140), plus recreating the target directory. Under sustained tool-call volume, this produces one full-state JSON write per call, serialized through a singlewriteQueue, which can accumulate I/O backlog.Debounce persistence (e.g. flush on a timer or after N updates) to reduce write amplification while keeping the opt-in telemetry file eventually consistent.
🤖 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 `@src/telemetry.ts` around lines 135 - 165, Batch telemetry persistence in persist and the recordToolCall/recordRetrieval paths instead of writing the full snapshot on every update. Add a debounce or update-count threshold so frequent calls coalesce into fewer directory, serialization, and file-write operations, while ensuring pending updates are eventually flushed and the existing writeQueue ordering and error handling remain intact.src/auth.ts (1)
31-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a more robust marker than a string prefix for validation errors.
resolveBrokerEndpointdistinguishes its own validation error from an arbitrary provider error by checkingerror.message.startsWith("oauth broker returned invalid endpoint:"). A provider implementation that throws anErrorwith a message starting with that exact literal string bypasses sanitization and its message reaches the caller unredacted.Use a dedicated error class or a non-enumerable marker property (e.g. a custom
MottainaiBrokerValidationError) instead of string matching, to make the distinction robust regardless of what message text a provider throws.🤖 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 `@src/auth.ts` around lines 31 - 44, Update resolveBrokerEndpoint to identify brokerUrl validation failures using a dedicated error type or non-enumerable marker rather than matching the error message prefix. Ensure only errors created by the broker validation path are rethrown unchanged, while provider errors—including ones with the same message prefix—are replaced with the sanitized resolution error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 6-8: Add a source entry module corresponding to the package's main
entry so the TypeScript build emits dist/index.js, preserving the existing main
and exports declarations; alternatively remove those package entry declarations
until a valid entry module exists.
In `@README.md`:
- Line 18: Update the ASCII diagram’s opening fenced code block in README.md to
specify the text language, changing the untyped fence to a text-labeled fence so
the MD040 requirement is satisfied.
In `@src/adaptive/trace.ts`:
- Around line 412-415: Update the trace store around loadTraces and
recordExecutionReview to maintain an in-memory set of execution IDs recorded
during the current store lifetime. In recordExecutionReview, use that set to
resolve same-store reviews without calling loadTraces; retain the loadTraces
fallback for IDs from other processes or reopened stores, and add each
successfully recorded execution ID to the set.
- Around line 361-390: Update the file-rotation flow in createTraceStore to
invoke sweepExpiredTraces again whenever a new trace file is created or rotated,
using the existing directory and retention-duration calculation from
prepareDirectory. Keep the prepared guard for initial directory setup, and
ensure subsequent rotations perform cleanup without recreating the directory
unnecessarily.
In `@src/compress/budget.ts`:
- Around line 12-38: Update compactToBudget so rawBytes below the envelope
reservation does not force the budget to the fixed 256-byte floor; retain the
target-based budget while applying only a non-negative rawBytes reservation.
Before calculating headBudget and selecting tail lines, reserve the omission
marker’s byte cost from the available budget, using that reduced value for both
head/tail splitting while preserving the existing marker format and output
behavior.
In `@src/compress/json.ts`:
- Around line 83-85: Validate the merged options in compressJsonValue before
calling compressValue, ensuring every numeric compression limit is finite,
non-negative, and an integer; reject invalid values or normalize them
consistently. Preserve the existing defaults and compression behavior for valid
inputs, and add regression coverage for negative maxStringLength, maxArrayItems,
and tailArrayItems.
- Around line 70-75: Update the object-building loop in compressValue to define
each key on out as an own data property, including "__proto__", instead of
assigning through out[key]. Preserve the existing compressed value and default
object prototype while ensuring the JSON key remains an enumerable own property.
In `@src/compress/lines.ts`:
- Around line 89-92: Update the line-retention logic in the compression function
around head, tail, and omitted so headLines is capped at maxTotalLines and
tailLines is capped to the remaining budget after the retained head. Preserve
normal behavior when the requested counts fit within the budget, and add a test
covering headLines + tailLines exceeding maxTotalLines without overlap or
negative omitted counts.
In `@src/compress/tool-description.ts`:
- Line 5: Update PROTECTED_LITERAL’s single-quoted literal branch to require
non-word-character boundaries before the opening apostrophe and after the
closing apostrophe, preventing apostrophes within contractions from forming
protected spans while preserving genuine quoted literals.
In `@src/envelope.ts`:
- Around line 27-30: Update the envelope construction around structuredContent
so details cannot overwrite reserved OUTPUT_SCHEMA fields, including operation,
status, summary, result_id, facts, diagnostics, metrics, and truncated. Validate
typed optional values from details, then remove reserved keys before spreading
only extension fields into the final structuredContent while preserving the
envelope defaults and error flag behavior.
In `@src/logging.ts`:
- Around line 63-77: Make serialization in boundedLogLine failure-tolerant for
both the full record and rawResult fallback hashing. Update the log flow around
boundedLogLine and log so synchronous JSON.stringify errors are caught like
writeQueue failures, recorded as a logging error, and never reject the caller’s
log() promise.
In `@src/read-governor/evidence.ts`:
- Around line 62-66: Validate retention options before assigning them in
InMemoryEvidenceStore’s constructor: require ttlMs to be finite and
non-negative, and maxEntries to be finite and a positive integer, rejecting
invalid values. Apply the same validation in src/retrieve.ts at lines 91-96 for
its constructor/options: ttlMs must be finite and non-negative, maxEntries
finite and positive integer, and the byte limit finite and positive.
- Around line 97-102: Update the read-evidence storage flow around issue() and
get() so the internal entries map retains a private record while both methods
return independent copies, preventing callers from mutating stored evidence
fields. Add a regression test that mutates the object returned by issue() and
verifies get() returns the original evidence values.
In `@src/retrieve.ts`:
- Around line 111-116: Update the artifact retention logic before entries.set so
maxBytes applies to the complete stored artifact, not only artifact.text.
Reserve space for the truncation footer, truncate text on UTF-8 boundaries
within the remaining budget, and bound stdout, stderr, and metadata rather than
retaining them through the object spread. Add tests covering oversized UTF-8
text and oversized stdout or stderr.
- Around line 136-142: Update the context calculation in the retrieval flow
around matchIndex, contextLines, and startLine so preceding context is capped at
maxLines - 1 before computing the window start, ensuring the matched line
remains in selected even when contextLines is large. Add a test covering
contextLines: 20 and maxLines: 1, verifying the returned window includes the
query match.
In `@src/state/migrations.ts`:
- Around line 68-82: Update applyMigrations to serialize discovery and
application: repeatedly start a BEGIN IMMEDIATE transaction before calling
currentVersion, select the next pending migration, and apply and record only
that migration within the transaction. Commit when successful, roll back on
failure, and repeat until no migration remains; preserve the existing migration
ordering and descriptive error behavior.
In `@src/state/paths.ts`:
- Around line 34-36: Update the XDG_STATE_HOME handling in the state-path
construction to accept the environment value only when it is non-empty and
absolute; otherwise fall back to path.join(home, ".local", "state"). Add a test
covering a relative XDG_STATE_HOME and verify the returned path uses the
home-directory fallback.
---
Nitpick comments:
In `@src/adaptive/trace.test.ts`:
- Around line 1-174: Extend the trace store tests with coverage for persistence
rotation and retention: add a test using MOTTAINAI_TRACE_MAX_FILE_BYTES with a
small limit that records enough data through createTraceStore to produce
multiple JSONL files, and add a test that ages a trace file with fs.utimesSync,
sets MOTTAINAI_TRACE_RETENTION_DAYS, then creates a new store and verifies the
aged file is removed. Anchor both tests near the existing createTraceStore
persistence tests and preserve current temporary-directory setup.
In `@src/auth.ts`:
- Around line 31-44: Update resolveBrokerEndpoint to identify brokerUrl
validation failures using a dedicated error type or non-enumerable marker rather
than matching the error message prefix. Ensure only errors created by the broker
validation path are rethrown unchanged, while provider errors—including ones
with the same message prefix—are replaced with the sanitized resolution error.
In `@src/telemetry.ts`:
- Around line 166-169: Update the snapshot() method to deep-clone the mutable
totals, by_provider, and by_capability counters before returning them, ensuring
callers cannot mutate the sink’s internal state while preserving the existing
snapshot metadata.
- Around line 135-165: Batch telemetry persistence in persist and the
recordToolCall/recordRetrieval paths instead of writing the full snapshot on
every update. Add a debounce or update-count threshold so frequent calls
coalesce into fewer directory, serialization, and file-write operations, while
ensuring pending updates are eventually flushed and the existing writeQueue
ordering and error handling remain intact.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b88263ca-bfe7-4fcd-89ed-bf42199c47f6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.editorconfig.gitignoreLICENSEREADME.mdmottainai.config.json.examplepackage.jsonpnpm-workspace.yamlsrc/adaptive/stats.test.tssrc/adaptive/stats.tssrc/adaptive/taxonomy.test.tssrc/adaptive/taxonomy.tssrc/adaptive/trace.test.tssrc/adaptive/trace.tssrc/auth.test.tssrc/auth.tssrc/compress/ansi.test.tssrc/compress/ansi.tssrc/compress/budget.test.tssrc/compress/budget.tssrc/compress/code.test.tssrc/compress/code.tssrc/compress/json.test.tssrc/compress/json.tssrc/compress/lines.test.tssrc/compress/lines.tssrc/compress/static-information.test.tssrc/compress/static-information.tssrc/compress/tool-description.test.tssrc/compress/tool-description.tssrc/envelope.tssrc/logging.test.tssrc/logging.tssrc/read-governor/classify.test.tssrc/read-governor/classify.tssrc/read-governor/evidence.test.tssrc/read-governor/evidence.tssrc/retrieve.test.tssrc/retrieve.tssrc/state/migrations.tssrc/state/paths.test.tssrc/state/paths.tssrc/state/store.tssrc/telemetry.test.tssrc/telemetry.tstsconfig.build.jsontsconfig.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
src/retrieve.ts (2)
255-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the retention-limit validation with the evidence store.
src/read-governor/evidence.tsLines 50-62 definevalidateTtlMsandvalidateMaxEntrieswith the same rules and the same messages. Move both helpers into a shared module and add amaxBytesvariant. The two stores then cannot drift apart.🤖 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 `@src/retrieve.ts` around lines 255 - 265, Extract the retention-limit validation currently duplicated in the retrieve store and the evidence store’s validateTtlMs and validateMaxEntries helpers into a shared module, preserving their existing rules and error messages. Add a shared validateMaxBytes helper with the same finite-positive-number validation, then update both stores to reuse all three shared validators instead of maintaining local implementations.
128-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared binary-search helper.
fitStringField,fitMetadataString, andfitTextimplement the same search loop with a different candidate builder. Extract one helper that takes abuild(bytes) => Tfunction and a predicate. One implementation then carries the monotonicity assumption and the off-by-one bounds.🤖 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 `@src/retrieve.ts` around lines 128 - 149, Extract the duplicated binary-search loop from fitStringField, fitMetadataString, and fitText into one shared helper accepting a build(bytes) => T function and a predicate that determines whether the candidate fits. Update each caller, including fitMetadataString, to provide its candidate builder and fit predicate while preserving the existing UTF-8 prefix behavior, byte limits, monotonic search, and returned best candidate.src/state/migrations.test.ts (1)
6-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a failing-migration test.
applyMigrationsrolls back the transaction and wraps the error with the migration version and description. No test covers that path. Add a case whereup()throws, then assert the error message and assert thatschema_migrationsdoes not record the failed version.🤖 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 `@src/state/migrations.test.ts` around lines 6 - 26, Add a test alongside the existing applyMigrations coverage where a migration’s up() throws, assert the thrown error includes its version and description, and verify schema_migrations contains no row for the failed version. Ensure the test closes the in-memory database and preserves the existing ordered-success behavior.src/read-governor/evidence.test.ts (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the per-entry
ttlMsvalidation.
issue()also validatesinput.ttlMsthroughvalidateTtlMs. No test exercises that path. Add one assertion so a future change toissue()cannot silently drop the check.♻️ Proposed additional assertion
assert.throws(() => new InMemoryEvidenceStore({ maxEntries: 0 }), /maxEntries/); }); + +test("issue() rejects an invalid per-entry ttlMs", () => { + const store = new InMemoryEvidenceStore(); + assert.throws(() => store.issue({ ...baseInput(), ttlMs: Number.POSITIVE_INFINITY }), /ttlMs/); + assert.throws(() => store.issue({ ...baseInput(), ttlMs: -1 }), /ttlMs/); +});🤖 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 `@src/read-governor/evidence.test.ts` around lines 60 - 66, Add an assertion in the “constructor rejects invalid retention limits” test or a nearby issue-validation test that calls InMemoryEvidenceStore.issue() with an invalid per-entry input.ttlMs and expects the /ttlMs/ validation error, covering the validateTtlMs path without changing existing constructor coverage.src/retrieve.test.ts (1)
82-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the stdout and stderr assertions.
Line 97 uses a disjunction. The test passes when only one field is shortened. Assert each field separately so a regression in either bounding path fails the test.
♻️ Proposed assertion change
- assert.ok(stdout.text.length < 100 || stderr.text.length < 1_000); + assert.ok(stdout.text.length < 100); + assert.ok(stderr.text.length < 1_000); + assert.equal(stdout.text.includes("\uFFFD"), false);🤖 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 `@src/retrieve.test.ts` around lines 82 - 98, Update the test “artifact store bounds oversized stdout and stderr fields” to assert truncation independently for stdout and stderr, replacing the combined disjunction with separate checks that each retrieved field is shorter than its original input. Keep the existing byte-length bounds unchanged.src/read-governor/evidence.ts (1)
112-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument
EvidenceStore.get()expiry semantics.
get()intentionally returns expired records. Authorization must compareexpiresAtwith the current time. Add this behavior to theEvidenceStore.get()contract.🤖 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 `@src/read-governor/evidence.ts` around lines 112 - 119, Document the expiry semantics in the EvidenceStore.get() contract: it intentionally returns records even when their expiresAt has passed, so callers must compare expiresAt against the current time for authorization decisions. Keep the existing get() behavior unchanged.src/adaptive/trace.test.ts (2)
99-116: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that fresh traces survive retention cleanup.
Line 115 checks only stale-file removal. Because
reopened.beginRequestwrites after preparation, a cleanup that deletes every existing JSONL file could still pass. Capture the request ID created byseedand verify thatreopened.load({ requestId })still returns it.🤖 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 `@src/adaptive/trace.test.ts` around lines 99 - 116, Update the trace retention test around seed and reopened.beginRequest to capture the request ID returned by seed, then assert reopened.load({ requestId }) still returns that seeded trace after cleanup. Keep the existing stalePath removal assertion so the test verifies both fresh-trace preservation and stale-file deletion.
91-97: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winVerify that rotation preserves all records.
Line 96 checks only the file count. A faulty rotation can create multiple files while losing or duplicating records. After
seedcompletes, load the trace and assert that the expected request, execution, and review data remains available.🤖 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 `@src/adaptive/trace.test.ts` around lines 91 - 97, Update the test “trace store rotates to multiple files at the configured size” to validate record preservation, not just file count. After seed completes, load the trace using the existing trace-loading API and assert that the expected request, execution, and review records are present exactly as expected, while retaining the multiple-files assertion.
🤖 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 `@src/envelope.ts`:
- Line 17: Update RESERVED_OUTPUT_FIELDS and the extension-copying logic in the
envelope construction flow to reserve isError alongside OUTPUT_SCHEMA fields,
preventing details.isError from being copied into structuredContent while
preserving the default result error flag behavior. Add a regression test
covering details: { isError: true } with the default isError parameter and
verify structuredContent excludes isError while the MCP result reports the
appropriate error state.
In `@src/retrieve.ts`:
- Around line 233-241: The payload-size reduction in the artifact-building flow
must preserve metadata.operation for search(). Update the truncation logic near
the metadata/stderr/stdout deletion loop to truncate text before dropping
metadata, or retain a minimal metadata record containing operation; keep
search() able to read entry.metadata?.operation while still enforcing maxBytes.
- Around line 85-124: Refactor fitStringField and the related fitText bounding
path to compute the payload’s fixed JSON overhead once using an empty target
field, derive the available byte budget arithmetically, and call utf8Prefix only
once per field. Use the resulting prefix directly when JSON escaping does not
consume additional capacity; retain the existing binary-search validation only
as a fallback for escaped characters, avoiding repeated full-payload
serialization and full-value Buffer allocation.
---
Nitpick comments:
In `@src/adaptive/trace.test.ts`:
- Around line 99-116: Update the trace retention test around seed and
reopened.beginRequest to capture the request ID returned by seed, then assert
reopened.load({ requestId }) still returns that seeded trace after cleanup. Keep
the existing stalePath removal assertion so the test verifies both fresh-trace
preservation and stale-file deletion.
- Around line 91-97: Update the test “trace store rotates to multiple files at
the configured size” to validate record preservation, not just file count. After
seed completes, load the trace using the existing trace-loading API and assert
that the expected request, execution, and review records are present exactly as
expected, while retaining the multiple-files assertion.
In `@src/read-governor/evidence.test.ts`:
- Around line 60-66: Add an assertion in the “constructor rejects invalid
retention limits” test or a nearby issue-validation test that calls
InMemoryEvidenceStore.issue() with an invalid per-entry input.ttlMs and expects
the /ttlMs/ validation error, covering the validateTtlMs path without changing
existing constructor coverage.
In `@src/read-governor/evidence.ts`:
- Around line 112-119: Document the expiry semantics in the EvidenceStore.get()
contract: it intentionally returns records even when their expiresAt has passed,
so callers must compare expiresAt against the current time for authorization
decisions. Keep the existing get() behavior unchanged.
In `@src/retrieve.test.ts`:
- Around line 82-98: Update the test “artifact store bounds oversized stdout and
stderr fields” to assert truncation independently for stdout and stderr,
replacing the combined disjunction with separate checks that each retrieved
field is shorter than its original input. Keep the existing byte-length bounds
unchanged.
In `@src/retrieve.ts`:
- Around line 255-265: Extract the retention-limit validation currently
duplicated in the retrieve store and the evidence store’s validateTtlMs and
validateMaxEntries helpers into a shared module, preserving their existing rules
and error messages. Add a shared validateMaxBytes helper with the same
finite-positive-number validation, then update both stores to reuse all three
shared validators instead of maintaining local implementations.
- Around line 128-149: Extract the duplicated binary-search loop from
fitStringField, fitMetadataString, and fitText into one shared helper accepting
a build(bytes) => T function and a predicate that determines whether the
candidate fits. Update each caller, including fitMetadataString, to provide its
candidate builder and fit predicate while preserving the existing UTF-8 prefix
behavior, byte limits, monotonic search, and returned best candidate.
In `@src/state/migrations.test.ts`:
- Around line 6-26: Add a test alongside the existing applyMigrations coverage
where a migration’s up() throws, assert the thrown error includes its version
and description, and verify schema_migrations contains no row for the failed
version. Ensure the test closes the in-memory database and preserves the
existing ordered-success behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0993dd3a-8c6e-4c79-95f9-ae6a6ff421bb
📒 Files selected for processing (27)
README.mdsrc/adaptive/trace.test.tssrc/adaptive/trace.tssrc/auth.test.tssrc/auth.tssrc/compress/budget.test.tssrc/compress/budget.tssrc/compress/json.test.tssrc/compress/json.tssrc/compress/lines.test.tssrc/compress/lines.tssrc/compress/tool-description.test.tssrc/compress/tool-description.tssrc/envelope.test.tssrc/envelope.tssrc/logging.test.tssrc/logging.tssrc/read-governor/evidence.test.tssrc/read-governor/evidence.tssrc/retrieve.test.tssrc/retrieve.tssrc/state/migrations.test.tssrc/state/migrations.tssrc/state/paths.test.tssrc/state/paths.tssrc/telemetry.test.tssrc/telemetry.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/auth.ts
- src/compress/budget.ts
- src/auth.test.ts
- src/state/paths.test.ts
- src/state/migrations.ts
- src/compress/tool-description.test.ts
- src/compress/tool-description.ts
- src/telemetry.test.ts
- src/logging.test.ts
- README.md
- src/compress/lines.ts
- src/state/paths.ts
- src/logging.ts
- src/adaptive/trace.ts
- envelope.ts: reserve `isError` alongside OUTPUT_SCHEMA fields so
details.isError can no longer leak into structuredContent while the
actual MCP result reports a different error state.
- retrieve.ts: keep a minimal { operation } metadata record alive
through artifact size bounding, reserving its bytes before text
truncation, so search() doesn't fall back to "unknown" once an
oversized text/stdout/stderr forces the rest of metadata out.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Prz9oz69aU1eZoVvcTry89
This was referenced Aug 3, 2026
This was referenced Aug 4, 2026
yohnark
pushed a commit
that referenced
this pull request
Aug 7, 2026
…lidation with CLI - callWorkflowCommandTool now checks config.workflowTasks before resolving defaultWorkflowStore() for task_start/task_status, instead of after. A disabled workspace no longer opens/initializes the on-disk SQLite DB before rejecting the call; the inner requireWorkflowTasksConfigured() checks stay as defense in depth. - Extracted validateTaskSlug/validateIssueRef from mcp-tools.ts's inline checks and reused them in src/cli.ts's `task start`, so the CLI rejects the same invalid taskSlug/issueRef values at the same boundary the MCP tool does, before any state reservation or git invocation (previously the CLI forwarded raw argv values straight into startTask). - Added a CLI test covering both rejections. Addresses blocker #1 and medium #3 from the human review on PR #69.
This was referenced Aug 8, 2026
4 tasks
This was referenced Aug 9, 2026
8 tasks
4 tasks
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
2 participants
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
First of a series of dependency-ordered import PRs from a private predecessor repository. This PR brings in only the foundation layer — files with no dependency on code that hasn't landed yet — so CI (install → typecheck → test → build) stays green at every step.
src/envelope.ts)src/logging.ts) and opt-in telemetry sink (src/telemetry.ts)src/retrieve.ts)src/auth.ts)src/state/paths.ts) and SQLite migration/store scaffoldingRenames from the predecessor
The predecessor project was named
mottainai-nosy-mcp. This PR renames its state-directory name and log-line prefixes tomottainai:src/state/paths.ts:APP_DIR_NAME→mottainai(wasmottainai-nosy-mcp)src/logging.ts,src/telemetry.ts,src/adaptive/trace.ts:console.errorprefixes updated to matchpackage.json: package name →@yohn-jp/mottainai, repository/homepage/bugs URLs →yohn-jp/mottainaiWhat's intentionally not here yet
proxy.ts,upstream.ts,config.ts,catalog.ts, adaptive routing, local tools, and the CLI entry point (index.ts) land in follow-up PRs as their prerequisite files arrive.README.mdis a minimal placeholder; the full architecture doc lands once the pieces it describes exist in this repo.package.jsonscripts/binentries referencing not-yet-present files (mcp,policy,read-governorscripts,mottainai/mtnaibin) are added back once those files land.Test plan
pnpm installpnpm run typecheck— passespnpm test— 111 passingpnpm run build— passes