chore(project): bootstrap Deno workspace and CI#16
Conversation
- Deno 2.x project structure with deno.json and task definitions - JSR dependencies: @cliffy/command, @std/assert, @std/testing, @std/yaml, @std/dotenv, @std/fs, @std/path - Full CLI command tree with stubs for all 15 issues - Shared interfaces (ProcessRunner, config types, ExitCode) for parallel work - FakeProcessRunner with recording, pre-programmed responses, and dry-run support - CI pipeline: fmt, lint, typecheck, test, coverage, and cross-platform build - .gitignore for generated and environment-specific files
wax911
left a comment
There was a problem hiding this comment.
Blocking review notes against #1:
-
.github/workflows/ci.ymlis not valid YAML. It starts with JavaScript-style block comments (/** ... */). GitHub Actions YAML comments must use#. As written, the workflow will not parse. -
The release/build tasks do not embed the permissions the compiled binary needs.
deno compileis called without explicit--allow-read,--allow-write,--allow-env, scoped--allow-sys, and scoped--allow-runpermissions. The compiled CLI will later need Docker/SOPS/age execution permissions baked in, per the issue requirements. -
The test task uses broad permissions (
--allow-run --allow-sys) rather than the explicit permission posture requested. That weakens the security model from the first commit. -
CI does not actually run
deno task coverage; it runs a separate broad-permissiondeno test ... --coverage=.coveragecommand. Keep the validation path aligned with the project tasks. -
The build job is effectively push-only because it is gated on branch refs. Pull requests will not validate
deno task build:linux:x64, despite that being part of the bootstrap acceptance criteria. -
CLI handlers call
Deno.exit()inside command registration actions. That makes the exportedmain(args): Promise<number>contract mostly pointless and makes tests harder because command execution can terminate the process. Return/throw controlled exit codes instead and keep process exiting at the true binary boundary.
Fix these before merging. This is the root PR for the chain, so downstream PRs inherit these problems.
wax911
left a comment
There was a problem hiding this comment.
Structural note for the full PR series:
The later PRs appear to be cumulative branches targeting main, not clean independent PRs or explicitly retargeted stacked PRs. Later feature PRs include earlier bootstrap/release/action files in their diff. This is only safe if they are merged strictly in order and every PR is rebased after its predecessor lands.
Safer options:
- Merge sequentially only: #16, then rebase #17, then #18, etc.
- Retarget stacked PRs so each PR targets its immediate predecessor branch.
- Mark downstream PRs as draft until their base dependency is merged.
Do not merge a later PR first. It can land the whole chain while only closing its own issue, which breaks traceability and makes review misleading.
- Fix CI YAML comments: replace JS-style /** */ with YAML # comments - Add scoped --allow-run permissions to all deno compile build tasks - Remove broad --allow-run --allow-sys from test task (Phase 1 tests need none) - Use 'deno task test --coverage' then 'deno task coverage' instead of separate command - Run build job on all PR triggers, not just push to main/dev - Replace Deno.exit() in action handlers with thrown Errors so main() controls exit
wax911
left a comment
There was a problem hiding this comment.
Follow-up review after the push:
Good fixes:
- The workflow file is now valid YAML.
- The build job now runs on PRs.
- CLI command handlers no longer call
Deno.exit()directly in this root PR. - Compile now scopes
--allow-runinstead of using broad--allow-run.
Remaining issues:
-
The latest CI run for this PR is still failing. From the visible workflow, a likely issue is that tests write coverage to
.coverageviadeno task test --coverage=.coverage, butdeno task coverageexpands todeno coverage --detailedwith no.coveragepath. Update the task todeno coverage --detailed .coverageor call the correct path in CI. -
Compile still uses unscoped
--allow-sys. The original acceptance criteria asked for scoped sys permissions rather than broad sys access. Use a bounded list such as--allow-sys=osRelease,uid,gid,hostname,userInfounless Deno rejects one of those names and the exception is documented. -
gitwas added to--allow-run, but it was not in the original required external command set. If the CLI needs Git, document why in the issue/README and keep it intentional; otherwise remove it.
#16 should not merge while CI is red.
Closes #1