refactor(cli): cover shared/functions with effect lint (CLI-2460)

[7ttp]

TL;DR

brings the shared functions runtime under the effect lint

whats introduced?

effect lint applied to the shared functions runtime:

• five allow list entries in .oxlintrc.effect.json for functions-docker, serve, their unit tests and the runtime watcher
• filesystem and path operations use injected FileSystem/Path services; streams preserve typed failures and JWT timestamps use Clock
• container config uses a schema codec; native errors retain existing diagnostics, exit codes and telemetry
• the watcher uses Effect Path, with a narrowly documented native fs import exception to preserve events, ordering and cleanup
• tests cover Windows path translation, native error compatibility and BOM-prefixed env files

ref:

• closes: CLI-2460

[github-actions]

Superseded by a newer AI review

🤖 AI Review

Eight findings are confirmed and two refuted. The main correctness issue is fs.watch acquisition throwing inside Effect.sync outside the expanded lint scope. Other confirmed concerns involve error metadata, secret-leak coverage, unmapped schema errors, raw tag checks, a throwing JWT path, generic error wrapping, and an unnecessary re-export.

Findings

Severity	Location	Category	Sources	Claim
🟡 MINOR	apps/cli/src/shared/functions/functions-docker.ts:406	error-handling	claude	nativePlatformFailure discards structured PlatformError metadata in its fallback, degrading diagnostics and telemetry classification.
🟡 MINOR	apps/cli/src/commands/functions/serve/serve.integration.test.ts:680	test-coverage	claude	The BOM test unnecessarily omits the secret-leak assertion for project-root environment files, leaving that security behavior unprotected.
🟡 MINOR	apps/cli/src/shared/runtime/file-watcher.service.ts:50	effect-safety	claude+codex	The extracted watcher implementation can throw during Effect.sync acquisition, producing a defect instead of FileWatcherError, while residing outside the Effect-lint allow list and the established service/layer split.
🟡 MINOR	apps/cli/src/shared/functions/serve.ts:1819	error-handling	claude	The new container-config schema encoder leaves SchemaError unmapped, so any encoding failure is misclassified as an API-response error.
🟡 MINOR	apps/cli/src/shared/functions/serve.ts:618	error-handling	claude	A user-supplied signing key with an unsupported algorithm throws inside the Effect workflow and becomes a defect rather than a typed configuration failure.
🟡 MINOR	apps/cli/src/shared/functions/functions-docker.ts:369	design	claude	NativeFailure is used as a transparent generic wrapper for distinct expected failures, leaving them without domain-specific classification despite the migration's typed-error requirement.
⚪ NIT	apps/cli/src/shared/functions/serve.ts:852	conventions	claude	New PlatformError branches use raw reason._tag comparisons instead of the repository's predicate helpers.
⚪ NIT	apps/cli/src/shared/functions/serve.ts:1	maintainability	claude	The watcher extraction retains an unnecessary alias re-export in serve.ts solely for one internal consumer.

Refuted findings (kept for transparency, not posted as review comments)

• apps/cli/src/shared/functions/functions-docker.unit.test.ts:83 (test-coverage): The Windows-host toDockerPath test is fully redundant with the injected Win32-layer test.
  Refuted: The tests exercise different providers: one tests the explicit Win32 implementation cross-platform, while the other verifies that the real host-selected production layer behaves correctly on Windows.
• apps/cli/src/shared/functions/serve.unit.test.ts:1 (conventions): The test improperly mixes @effect/vitest and vitest imports.
  Refuted: This is an established repository pattern: pgdelta-next-adapter.unit.test.ts, shared/compute/tar.unit.test.ts, and several other tests import effect-aware it from @effect/vitest while retaining describe and expect from vitest. No trusted convention prohibits it.

Stats

Claude findings: 10 · Codex findings: 1 · Confirmed: 8 · Refuted: 2 · Uncertain: 0

---

Models: claude-opus-5 + gpt-5.6-sol · Trigger: auto · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.

[github-actions]

🤖 AI Review

Adjudicated all 11 reported findings against the checked-out PR and trusted conventions. Eight are confirmed: five minor correctness/robustness concerns and three nits. Three style/design findings are refuted by type invariants or documented boundary/comment conventions. No critical or major issue was verified.

Findings

Severity	Location	Category	Sources	Claim
🟡 MINOR	apps/cli/src/shared/runtime/file-watcher.service.ts:45	lint-scope	claude	The native watcher implementation was moved out of newly Effect-linted serve.ts into a file excluded from Effect lint, bypassing the migration's intended lint coverage.
🟡 MINOR	apps/cli/src/shared/functions/serve.ts:1819	error-handling	claude	The new functions-container encoder exposes an unmapped SchemaError from local container setup, which telemetry misclassifies as an API-response failure.
🟡 MINOR	apps/cli/src/shared/functions/functions-docker.ts:406	error-handling	claude	For cause-less PlatformErrors, nativePlatformFailure stringifies the reason payload instead of preserving its useful description or the PlatformError message.
🟡 MINOR	apps/cli/src/shared/functions/functions-docker.ts:400	robustness	claude	Cause-less filesystem argument errors are classified by parsing the mutable human-readable description text.
🟡 MINOR	apps/cli/src/shared/runtime/file-watcher.service.ts:50	error-handling	codex	Synchronous fs.watch acquisition failures become defects rather than the stream's declared FileWatcherError.
⚪ NIT	apps/cli/src/commands/functions/serve/serve.integration.test.ts:680	test-coverage	claude	The BOM test omits its secret-nonleakage assertion specifically for the project-level environment-file path without explaining the exemption.
⚪ NIT	.oxlintrc.effect.json:13	style	claude	The new Effect-lint entries disrupt the configuration's existing package/command/shared/test grouping and introduce the list's only blank separator.
⚪ NIT	apps/cli/src/shared/functions/serve.ts:852	effect-conventions	codex	New PlatformError handling inspects raw _tag fields instead of using Effect's public predicate helpers.

Refuted findings (kept for transparency, not posted as review comments)

• apps/cli/src/shared/telemetry/error-actionability.ts:856 (coupling): NativeFailure is improperly coupled across modules because telemetry identifies it through its stable tag rather than importing its class.
  Refuted: Error normalization and telemetry are genuinely dynamic boundaries, for which trusted AGENTS.md permits tag-based predicates. The tag is also the repository's explicitly stable telemetry identity, and importing the class back into telemetry would create a circular dependency with functions-docker.ts:6-11.
• apps/cli/src/shared/telemetry/error-actionability.ts:859 (error-handling): Returning undefined for a NativeFailure with a non-Error cause discards diagnostics from valid wrappers.
  Refuted: A non-Error cause violates the class's declared invariant and cannot be produced by its factory. The undefined fallback is defensive handling for malformed or cyclic wrappers; returning the wrapper would also recurse through NativeFailure.message.
• apps/cli/src/shared/functions/serve.ts:1 (style): The line-one re-export requires relocation and a comment explaining that its implementation moved.
  Refuted: The re-export is valid ESM, passes the repository's source organization, and its module path already identifies the implementation. The trusted comment policy explicitly rejects provenance/history comments such as explaining that code was moved.

Stats

Claude findings: 9 · Codex findings: 2 · Confirmed: 8 · Refuted: 3 · Uncertain: 0

---

Models: claude-opus-5 + gpt-5.6-sol · Trigger: manual · Workflow run

This review runs once per PR. A maintainer can request another with a /ai-review comment.