Repository navigation
feat(#115): cold-installed public role matrix (owner README gate remaining) - #175
Conversation
Drop deferred-slice stubs now that all seven public roles dispatch, prove one isolated Pi install + package update on the real ak-role bin, and document ak-role-only invocation/result reading for the release candidate.
Root causes on default npm test: 1 Reviewer package-lifecycle still expanded home code-review while the installed runtime binds package resources/methods/code-review — capture failed closed (swallowed), so compliance audit never received structured_execution_record. Point the oracle at the packaged skill. 2) Authentic-cut #45 lost its no-session run when stderr/recorder trim deleted the only tracked files; empty run dirs are not in git, so the ticket fell to pending. Restore the authentic no-session run bytes. npm test: 582 pass / 0 fail. No push. EOF )
Reviewer's GuideCompletes the public Flow diagram for ak-role command dispatch after cold-install matrix completionflowchart TD
User(["User runs ak-role <command>"]) --> runAkRole
runAkRole["runAkRole(argv, env)"] --> parseCommand
parseCommand["parsed.command"] -->|matches PUBLIC_CALLABLE_ROLE| roleHandler
parseCommand -->|no matching handler| cliUsageError
roleHandler["role-specific handler (judge/coder/fixer/collector/doctor/reviewer/merger)"] --> normalExit["return { exitCode: 0, terminal? }"]
cliUsageError["throw CliUsageError('unknown command: ' + parsed.command)"] --> errorHandling["CLI error handling & exit"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_845b5037-e03e-41ba-a1f0-5c422ab7f7a2) |
📝 WalkthroughWalkthroughThe public CLI now dispatches all callable roles without deferred-role fallback handling. README guidance covers package updates, Navigator, and direct role invocation. Packaging tests validate role reachability, packaged resources, cold installation, package updates, and skill isolation. ChangesPublic CLI release
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PackageTarball
participant Pi
participant PublicCLI
participant RoleRuntime
PackageTarball->>Pi: install versioned package
Pi->>PublicCLI: expose installed executable
PublicCLI->>RoleRuntime: load packaged runtime resources
PublicCLI-->>Pi: report role discovery and results
PackageTarball->>Pi: replace stable tarball
Pi->>PublicCLI: run documented package update
PublicCLI->>RoleRuntime: use updated shared runtime
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
public-cli-cold-matrix.test.tsis quite large and mixes a lot of responsibilities (install/update semantics, resource shape, per-role behavior); consider splitting it into smaller focused tests or extracting shared helpers to keep individual test cases easier to understand and maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `public-cli-cold-matrix.test.ts` is quite large and mixes a lot of responsibilities (install/update semantics, resource shape, per-role behavior); consider splitting it into smaller focused tests or extracting shared helpers to keep individual test cases easier to understand and maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 887bf42f78
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the shell activation path — both are completed public paths. | ||
| try { | ||
| const args = JSON.parse(await readFile(argvLog, "utf8")) as string[]; | ||
| if (flagValue(args, "--ak-role") === "merger") { |
There was a problem hiding this comment.
When this seeded project has no active merge, Merger fails before invoking the shim, so argvLog still contains Doctor's arguments from the preceding block. The read succeeds, this condition is false, the catch does not run, and result.code is never checked; therefore even an unknown-command or otherwise broken installed Merger path passes the advertised seven-role cold-install matrix. Remove the stale log before invocation and explicitly assert either a fresh Merger dispatch or the expected controlled activation failure.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
test/package/public-cli-bin-artifact.test.ts (1)
172-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the role list from the registry and drop the duplicated fixer/doctor dispatch checks.
Two points on this block:
- Duplication. The loop at lines 198-208 checks the same
command === "<role>" | case "<role>"pattern forfixeranddoctorthat lines 173-177 and 183-187 already check. Keep the role-specific assertions that the loop does not cover: thediagnosing-bugsskill path and theak-doctor-caseflag.- Drift. The role list is a literal.
test/package/public-cli-cold-matrix.test.tsimportsPUBLIC_CALLABLE_ROLESand asserts a length of 7 againstPACKAGED_ROLE_REGISTRY. If a packaged role is added, this artifact test will not require its dispatch path in the shipped bin. Import the same constant here.Note on the static analysis hint for line 194:
roleholds only literals declared in this file, so thenew RegExpconstruction carries no untrusted input.♻️ Proposed consolidation
- // `#110` / `#113` public Fixer + Doctor adapters must ship in the installed bin. - assert.equal( - /command\s*===\s*["']fixer["']|case\s*["']fixer["']/.test(committedText), - true, - "public bin must ship fixer command dispatch", - ); + // `#110` / `#113` public Fixer + Doctor adapters must ship in the installed bin. assert.equal( /resolvePackagedMethodSkillPath\([^)]*"diagnosing-bugs"/.test(committedText), true, "public bin must resolve the package-owned diagnosing-bugs skill path", ); - assert.equal( - /command\s*===\s*["']doctor["']|case\s*["']doctor["']/.test(committedText), - true, - "public bin must ship doctor command dispatch", - ); assert.equal( committedText.includes("ak-doctor-case"), true, "public bin must pin doctor case flag on activation", ); // `#115`: every public callable role is a completed dispatch path in the shipped bin. const roleDispatch = (role: string): boolean => new RegExp( "command\\s*===\\s*[\"']" + role + "[\"']|case\\s*[\"']" + role + "[\"']", ).test(committedText); - for (const role of [ - "judge", - "coder", - "fixer", - "reviewer", - "collector", - "doctor", - "merger", - ] as const) { + for (const role of PUBLIC_CALLABLE_ROLES) { assert.equal(roleDispatch(role), true, `public bin must ship ${role} command dispatch`); }Add the import at the top of the file:
import { PUBLIC_CALLABLE_ROLES } from "../../src/public-cli/registry.ts";🤖 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/package/public-cli-bin-artifact.test.ts` around lines 172 - 208, Update the public bin artifact test to import and iterate over PUBLIC_CALLABLE_ROLES from src/public-cli/registry.ts instead of maintaining a duplicated literal role list. Remove the standalone fixer and doctor dispatch assertions, while retaining the diagnosing-bugs skill-path and ak-doctor-case flag assertions and the existing roleDispatch validation.test/package/reviewer-package-lifecycle.test.ts (1)
30-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe comment claims precedence over an ambient skill that the test no longer exercises.
Line 31 states that an ambient home
code-reviewmust not take/skill:code-reviewexpansion from the package binding. The setup no longer creates an ambientcode-reviewskill, becausewriteTestSkillwas dropped from the imports on line 10. The test now only reads the skill frominstalledRoot. It passes whether or not the package binding wins over an ambient competitor, so it does not prove the precedence claim.Choose one of two options:
- Restore the competitor. Write an ambient
code-reviewskill under the hermetic home with distinct content, then assert that the resolved expansion matches the packaged file.- Narrow the comment. State that the home is empty, and add an explicit assertion that no ambient skills tree exists, in the style of
test/package/public-cli-cold-matrix.test.tslines 369-372.♻️ Option 2: assert the empty ambient tree instead of implying precedence
- // Package-owned code-review (`#111`): empty home; skill path from installed package tree. - // Ambient home code-review must not steal /skill:code-review expansion from the package binding. + // Package-owned code-review (`#111`): the ambient home carries no skills tree, + // so the resolved skill path can only come from the installed package tree. await withColdInstalledPackage(home, async ({ fixture, pack, installedRoot }) => { + await assert.rejects( + () => access(resolve(home, ".agents", "skills")), + (error: NodeJS.ErrnoException) => error.code === "ENOENT", + ); assert.ok(pack.files.some((file) => file.path === "src/reviewer-dispatch.ts"));🤖 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/package/reviewer-package-lifecycle.test.ts` around lines 30 - 38, Update the test around withColdInstalledPackage to match its actual coverage: either restore an ambient code-review competitor with distinct content and assert the resolved /skill:code-review expansion uses the packaged skill, or narrow the comment to the empty-home behavior and explicitly assert that no ambient skills tree exists, following the existing public CLI cold-matrix pattern.test/unit/public-cli-cli.test.ts (1)
140-177: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the assertion so a missing dispatch path fails the test.
This test is the unit guard for the removed deferred-role fallback in
src/public-cli/cli.ts. The current assertions do not close that gap. If a role were dropped from the dispatcher,runAkRolereachesthrow new CliUsageError("unknown command: <role>"). That returns exit code 2 and prints no deferred-slice prose, so bothassert.notEqual(result.exitCode, 0, role)and the two substring assertions still pass.Add an assertion that the failure is not a structural reject of the command token itself.
♻️ Proposed assertion for the dispatch invariant
assert.notEqual(result.exitCode, 0, role); + // A dropped dispatch path would surface as `unknown command: <role>`. + assert.equal( + `${stdout.join("")}${stderr.join("")}`.includes(`unknown command: ${role}`), + false, + role, + ); assert.equal( stderr.join("").includes("not available in this install slice"),🤖 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/public-cli-cli.test.ts` around lines 140 - 177, Strengthen the test around runAkRole so each PUBLIC_CALLABLE_ROLES entry must not fail as an unknown command or otherwise structurally reject the role token. Add an assertion against the returned result or captured output that specifically excludes the “unknown command” dispatch failure, while preserving the existing nonzero-exit and deferred-slice checks.test/package/public-cli-cold-matrix.test.ts (1)
309-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the harness installer instead of duplicating it.
installFromTarballrepeatsinstallPackedArtifactIntoPiNpmfromtest/helpers/pi-test-harness.tslines 582-623. The only difference is the tarball source. Add an optional tarball parameter to the harness helper, then call it from this test. That keeps thePiManagedInstallshape and thepi installerror handling in one place.Also replace
tarball.split("/").pop()!on line 350 withbasename(tarball).basenamecomes fromnode:path, which this file already imports, and it removes the non-null assertion.♻️ Local part of the change
-import { dirname, join, resolve } from "node:path"; +import { basename, dirname, join, resolve } from "node:path";pack: { root: dirname(tarball), tarball, - filename: tarball.split("/").pop()!, + filename: basename(tarball), files: [], },🤖 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/package/public-cli-cold-matrix.test.ts` around lines 309 - 354, Extend installPackedArtifactIntoPiNpm with an optional tarball parameter, using it to build the file-based npm source when provided, then replace installFromTarball with a call to that shared helper while preserving its existing inputs and behavior. In the returned pack metadata, use the existing node:path basename import for the tarball filename instead of splitting the path and asserting non-null.src/public-cli/cli.ts (1)
891-893: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a compile-time guard for the handler invariant.
The comment states that every
PUBLIC_CALLABLE_ROLEhas a handler above. Nothing enforces this at compile time. IfPACKAGED_ROLE_REGISTRYgains a role, the new role falls through tounknown commandat runtime, and the tests that assert exit code 2 for unknown tokens will not distinguish it.A narrowed-type assertion before the throw makes the invariant fail at build time instead.
♻️ Sketch of a compile-time exhaustiveness guard
- // `#115`: every PUBLIC_CALLABLE_ROLE has a completed handler above. Unknown - // tokens (including misspelled role names) are structural rejects. + // `#115`: every PUBLIC_CALLABLE_ROLE has a completed handler above. Unknown + // tokens (including misspelled role names) are structural rejects. + // Compile-time proof: `parsed.command` can no longer be a callable role. + const _exhaustive: Exclude<typeof parsed.command, PackagedRole> = parsed.command; + void _exhaustive; throw new CliUsageError(`unknown command: ${parsed.command}`);🤖 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/public-cli/cli.ts` around lines 891 - 893, In the command-dispatch logic immediately before the `CliUsageError` throw, add a narrowed-type exhaustiveness assertion for `parsed.command` covering every `PUBLIC_CALLABLE_ROLE` from `PACKAGED_ROLE_REGISTRY`. Keep the unknown-token rejection behavior unchanged while ensuring any newly added callable role that lacks a handler causes a compile-time failure.test/fixtures/factory-board-kanban/ledger/issues/45/runs/final-authority-judge@ak-pi-workflow-roles-issue44/recorder-config.json (1)
4-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused recorder capture or make it a seam-owned fixture.
test/contract/factory-board.test.tshashes this file but does not parse it.src/ticket-trajectory.tsreads only session*.jsonlfiles andinvocation.json. Remove the file, or add a focused test with portable paths and the correctissue45directory identity.🤖 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/fixtures/factory-board-kanban/ledger/issues/45/runs/final-authority-judge`@ak-pi-workflow-roles-issue44/recorder-config.json around lines 4 - 13, The recorder-config.json fixture is unused by runtime code and only contributes to contract hashing. Remove this fixture and update the affected contract fixture expectations, or make it seam-owned by adding a focused test that uses portable paths and the correct issue45 directory identity. Ensure no references remain inconsistent with test/contract/factory-board.test.ts and src/ticket-trajectory.ts.Source: Coding guidelines
🤖 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 `@README.md`:
- Around line 350-352: Update the public invocation paragraph in README.md to
retain the ak-role collector guidance while removing the `#78` ledger-book
reference and persistent correlated-session behavior. Keep internal storage and
persistence details out of the public contract.
In `@test/package/public-cli-cold-matrix.test.ts`:
- Around line 128-153: Update runAkRoleBin’s spawned child handling to register
an error listener alongside the existing stdout, stderr, and close listeners. On
spawn failure, clear the timeout and resolve the result using the same
error-handling pattern as the generated shim, preserving the existing result
shape so the test can assert the failure without terminating the process.
- Around line 616-636: Update the argv-log verification around the JSON parsing
block so assertion failures from the merger checks propagate and fail the test.
Delete argvLog before invoking the command, then branch on whether the log
exists: parse and validate it when present, or otherwise assert the expected
nonzero result and deferred-prose contract for the invocation failure.
- Around line 162-199: Update the extensionless pi shim generated in the forward
branch of the test helper to use an explicit module format compatible with Node
20.0–20.18, replacing the ESM import syntax with CommonJS or another explicit
.mjs implementation. Preserve argvLog writing, child spawning, and exit/signal
propagation behavior.
---
Nitpick comments:
In `@src/public-cli/cli.ts`:
- Around line 891-893: In the command-dispatch logic immediately before the
`CliUsageError` throw, add a narrowed-type exhaustiveness assertion for
`parsed.command` covering every `PUBLIC_CALLABLE_ROLE` from
`PACKAGED_ROLE_REGISTRY`. Keep the unknown-token rejection behavior unchanged
while ensuring any newly added callable role that lacks a handler causes a
compile-time failure.
In
`@test/fixtures/factory-board-kanban/ledger/issues/45/runs/final-authority-judge`@ak-pi-workflow-roles-issue44/recorder-config.json:
- Around line 4-13: The recorder-config.json fixture is unused by runtime code
and only contributes to contract hashing. Remove this fixture and update the
affected contract fixture expectations, or make it seam-owned by adding a
focused test that uses portable paths and the correct issue45 directory
identity. Ensure no references remain inconsistent with
test/contract/factory-board.test.ts and src/ticket-trajectory.ts.
In `@test/package/public-cli-bin-artifact.test.ts`:
- Around line 172-208: Update the public bin artifact test to import and iterate
over PUBLIC_CALLABLE_ROLES from src/public-cli/registry.ts instead of
maintaining a duplicated literal role list. Remove the standalone fixer and
doctor dispatch assertions, while retaining the diagnosing-bugs skill-path and
ak-doctor-case flag assertions and the existing roleDispatch validation.
In `@test/package/public-cli-cold-matrix.test.ts`:
- Around line 309-354: Extend installPackedArtifactIntoPiNpm with an optional
tarball parameter, using it to build the file-based npm source when provided,
then replace installFromTarball with a call to that shared helper while
preserving its existing inputs and behavior. In the returned pack metadata, use
the existing node:path basename import for the tarball filename instead of
splitting the path and asserting non-null.
In `@test/package/reviewer-package-lifecycle.test.ts`:
- Around line 30-38: Update the test around withColdInstalledPackage to match
its actual coverage: either restore an ambient code-review competitor with
distinct content and assert the resolved /skill:code-review expansion uses the
packaged skill, or narrow the comment to the empty-home behavior and explicitly
assert that no ambient skills tree exists, following the existing public CLI
cold-matrix pattern.
In `@test/unit/public-cli-cli.test.ts`:
- Around line 140-177: Strengthen the test around runAkRole so each
PUBLIC_CALLABLE_ROLES entry must not fail as an unknown command or otherwise
structurally reject the role token. Add an assertion against the returned result
or captured output that specifically excludes the “unknown command” dispatch
failure, while preserving the existing nonzero-exit and deferred-slice checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa9869ee-1747-47cc-9388-b315d10cd421
⛔ Files ignored due to path filters (2)
dist/public-cli/main.jsis excluded by!**/dist/**test/fixtures/factory-board-kanban/ledger/issues/45/runs/final-authority-judge@ak-pi-workflow-roles-issue44/stderr.logis excluded by!**/*.log
📒 Files selected for processing (7)
README.mdsrc/public-cli/cli.tstest/fixtures/factory-board-kanban/ledger/issues/45/runs/final-authority-judge@ak-pi-workflow-roles-issue44/recorder-config.jsontest/package/public-cli-bin-artifact.test.tstest/package/public-cli-cold-matrix.test.tstest/package/reviewer-package-lifecycle.test.tstest/unit/public-cli-cli.test.ts
| The public adapter accepts a PR, repository identity, and explicit leg/expected-author declarations, then assembles Collector's retained manifest. Callers do not construct the internal manifest themselves. | ||
|
|
||
| > **Public invocation:** use `ak-role collector` (see **Call Collector** above). Collector keeps a persistent correlated session under the #78 ledger book; the former public-looking `--no-session` recipe was incorrect and has been removed. | ||
| > **Public invocation:** use `ak-role collector` (see **Call Collector** above). Collector keeps a persistent correlated session under the #78 ledger book. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the internal ledger-book reference from public documentation.
The new sentence makes the #78 ledger book and persistent session behavior part of the public contract. The PR objective requires storage layouts to remain non-contractual. Keep the ak-role collector invocation guidance and remove the internal persistence detail.
Proposed documentation change
-> **Public invocation:** use `ak-role collector` (see **Call Collector** above). Collector keeps a persistent correlated session under the `#78` ledger book.
+> **Public invocation:** use `ak-role collector` (see **Call Collector** above).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The public adapter accepts a PR, repository identity, and explicit leg/expected-author declarations, then assembles Collector's retained manifest. Callers do not construct the internal manifest themselves. | |
| > **Public invocation:** use `ak-role collector` (see **Call Collector** above). Collector keeps a persistent correlated session under the #78 ledger book; the former public-looking `--no-session` recipe was incorrect and has been removed. | |
| > **Public invocation:** use `ak-role collector` (see **Call Collector** above). Collector keeps a persistent correlated session under the #78 ledger book. | |
| The public adapter accepts a PR, repository identity, and explicit leg/expected-author declarations, then assembles Collector's retained manifest. Callers do not construct the internal manifest themselves. | |
| > **Public invocation:** use `ak-role collector` (see **Call Collector** above). |
🤖 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 `@README.md` around lines 350 - 352, Update the public invocation paragraph in
README.md to retain the ak-role collector guidance while removing the `#78`
ledger-book reference and persistent correlated-session behavior. Keep internal
storage and persistence details out of the public contract.
| const child = spawn(bin, args, { | ||
| cwd: options.cwd ?? options.home, | ||
| env: { | ||
| ...mergedEnv, | ||
| PATH: pathPrefix, | ||
| }, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
| let stdout = ""; | ||
| let stderr = ""; | ||
| let timedOut = false; | ||
| const timer = setTimeout(() => { | ||
| timedOut = true; | ||
| child.kill("SIGKILL"); | ||
| }, options.timeoutMs ?? 45_000); | ||
| child.stdout.on("data", (chunk) => { | ||
| stdout += String(chunk); | ||
| }); | ||
| child.stderr.on("data", (chunk) => { | ||
| stderr += String(chunk); | ||
| }); | ||
| child.on("close", (code) => { | ||
| clearTimeout(timer); | ||
| resolvePromise({ code, stdout, stderr, timedOut }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add an error listener to the spawned child.
runAkRoleBin registers listeners for stdout, stderr, and close only. If spawn cannot start bin, the child emits error. A ChildProcess is an EventEmitter, so an error event with no listener throws and terminates the test process instead of failing an assertion. The pending timer also leaks in that case.
The generated shim at lines 178-181 already registers an error handler. Apply the same pattern here.
🛡️ Proposed fix
let timedOut = false;
const timer = setTimeout(() => {
timedOut = true;
child.kill("SIGKILL");
}, options.timeoutMs ?? 45_000);
+ child.on("error", (error) => {
+ clearTimeout(timer);
+ resolvePromise({
+ code: null,
+ stdout,
+ stderr: `${stderr}spawn failed: ${String(error)}`,
+ timedOut,
+ });
+ });
child.stdout.on("data", (chunk) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const child = spawn(bin, args, { | |
| cwd: options.cwd ?? options.home, | |
| env: { | |
| ...mergedEnv, | |
| PATH: pathPrefix, | |
| }, | |
| stdio: ["ignore", "pipe", "pipe"], | |
| }); | |
| let stdout = ""; | |
| let stderr = ""; | |
| let timedOut = false; | |
| const timer = setTimeout(() => { | |
| timedOut = true; | |
| child.kill("SIGKILL"); | |
| }, options.timeoutMs ?? 45_000); | |
| child.stdout.on("data", (chunk) => { | |
| stdout += String(chunk); | |
| }); | |
| child.stderr.on("data", (chunk) => { | |
| stderr += String(chunk); | |
| }); | |
| child.on("close", (code) => { | |
| clearTimeout(timer); | |
| resolvePromise({ code, stdout, stderr, timedOut }); | |
| }); | |
| }); | |
| const child = spawn(bin, args, { | |
| cwd: options.cwd ?? options.home, | |
| env: { | |
| ...mergedEnv, | |
| PATH: pathPrefix, | |
| }, | |
| stdio: ["ignore", "pipe", "pipe"], | |
| }); | |
| let stdout = ""; | |
| let stderr = ""; | |
| let timedOut = false; | |
| const timer = setTimeout(() => { | |
| timedOut = true; | |
| child.kill("SIGKILL"); | |
| }, options.timeoutMs ?? 45_000); | |
| child.on("error", (error) => { | |
| clearTimeout(timer); | |
| resolvePromise({ | |
| code: null, | |
| stdout, | |
| stderr: `${stderr}spawn failed: ${String(error)}`, | |
| timedOut, | |
| }); | |
| }); | |
| child.stdout.on("data", (chunk) => { | |
| stdout += String(chunk); | |
| }); | |
| child.stderr.on("data", (chunk) => { | |
| stderr += String(chunk); | |
| }); | |
| child.on("close", (code) => { | |
| clearTimeout(timer); | |
| resolvePromise({ code, stdout, stderr, timedOut }); | |
| }); | |
| }); |
🤖 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/package/public-cli-cold-matrix.test.ts` around lines 128 - 153, Update
runAkRoleBin’s spawned child handling to register an error listener alongside
the existing stdout, stderr, and close listeners. On spawn failure, clear the
timeout and resolve the result using the same error-handling pattern as the
generated shim, preserving the existing result shape so the test can assert the
failure without terminating the process.
| const shimPath = resolve(shimDir, "pi"); | ||
| const realPi = await realpath(piCli); | ||
| const forward = options.forward === true; | ||
| const exitCode = options.exitCode ?? 1; | ||
| if (forward) { | ||
| await writeFile( | ||
| shimPath, | ||
| `#!/usr/bin/env node | ||
| import { writeFileSync } from "node:fs"; | ||
| import { spawn } from "node:child_process"; | ||
| const args = process.argv.slice(2); | ||
| writeFileSync(${JSON.stringify(argvLog)}, JSON.stringify(args), "utf8"); | ||
| const child = spawn(${JSON.stringify(realPi)}, args, { | ||
| stdio: "inherit", | ||
| env: process.env, | ||
| }); | ||
| child.on("error", (error) => { | ||
| console.error(error); | ||
| process.exit(1); | ||
| }); | ||
| child.on("close", (code, signal) => { | ||
| if (signal) process.kill(process.pid, signal); | ||
| process.exit(code ?? 1); | ||
| }); | ||
| `, | ||
| "utf8", | ||
| ); | ||
| } else { | ||
| await writeFile( | ||
| shimPath, | ||
| `#!/usr/bin/env node | ||
| import { writeFileSync } from "node:fs"; | ||
| writeFileSync(${JSON.stringify(argvLog)}, JSON.stringify(process.argv.slice(2)), "utf8"); | ||
| process.exit(${exitCode}); | ||
| `, | ||
| "utf8", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Report the declared Node engines/target and CI Node versions.
set -uo pipefail
echo "== engines / type in root package.json =="
fd -d 1 '^package.json$' . --exec jq '{name, type, engines, packageManager}'
echo "== node version pins =="
fd -H -d 2 '^(\.nvmrc|\.node-version|\.tool-versions)$' --exec sh -c 'echo "--- $1"; cat "$1"' sh {}
echo "== CI node-version declarations =="
rg -n 'node-version|setup-node|FROM node' .github 2>/dev/null
echo "== other extensionless node shims in tests, for precedent =="
rg -n '#!/usr/bin/env node' test 2>/dev/null -A2Repository: Akagilnc/ak-pi-workflow-roles
Length of output: 3718
🌐 Web query:
Node.js extensionless entry point CommonJS ESM syntax detection default version
💡 Result:
In Node.js, the module system for an extensionless entry point or file is determined by the nearest parent package.json file's "type" field [1][2]. If no "type" field is present, Node.js employs an automatic syntax detection mechanism to decide whether to load the file as CommonJS or as an ES module [3][1]. This syntax detection feature is enabled by default [4]. When Node.js encounters an ambiguous file (i.e., one with a.js or no extension in a context lacking an explicit "type" field), it initially attempts to parse the file as CommonJS [4]. If that parsing fails due to the presence of ES module syntax (such as import or export statements), Node.js automatically retries and treats the file as an ES module [3][4][5]. Key takeaways: - Explicit Type: If the nearest package.json defines "type": "module" or "type": "commonjs", the extensionless file will be treated as that format regardless of its contents [6][1][7]. - Syntax Detection: In the absence of an explicit "type" field, the system performs dynamic analysis [3][8][4]. If ES module syntax is detected, the file is run as an ES module; otherwise, it defaults to CommonJS [3][5]. - Performance: While syntax detection is enabled by default, it can introduce a minor performance penalty for ES modules. To avoid this, it is recommended to explicitly set "type": "module" in your package.json [4]. This behavior ensures that extensionless files—often used for CLI tools or scripts—can safely contain ES module syntax without requiring an explicit extension or project-level configuration [8][4].
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/modules.html
- 2: https://nodejs.org/api/modules.html
- 3: https://nodejs.org/api/esm.html
- 4: module: unflag detect-module nodejs/node#53619
- 5: http://nodejs.org/docs/latest/api/esm.html
- 6: doc: remove extensionless CJS exception for type:module nodejs/node#62176
- 7: https://nodejs.org/api/cli.html
- 8: esm: detect ESM syntax in ambiguous JavaScript nodejs/node#50096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== local Node runtime =="
node --version
echo "== extensionless shim behavior in an empty temporary directory =="
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
cat >"$tmp/pi" <<'JS'
import { writeFileSync } from "node:fs";
writeFileSync(process.argv[2], "ok", "utf8");
JS
node "$tmp/pi" "$tmp/result"
if [ -f "$tmp/result" ]; then
echo "extensionless-import: runs"
else
echo "extensionless-import: no-result"
fi
echo "== relevant test error handling =="
sed -n '600,650p' test/package/public-cli-cold-matrix.test.tsRepository: Akagilnc/ak-pi-workflow-roles
Length of output: 2232
🌐 Web query:
Node.js 20 extensionless files import syntax detection enabled by default exact release version
💡 Result:
Module syntax detection for ambiguous files (including those without file extensions) was enabled by default in Node.js version 20.19.0 [1][2][3]. This feature, which was previously available behind the --experimental-detect-module flag [4][5], allows Node.js to inspect the source code of files that lack a "type" field in the nearest parent package.json file [1][6]. If ES module syntax (such as import, export, or top-level await) is detected in an ambiguous file, Node.js will execute it as an ES module rather than CommonJS [1][6][3]. Users who wish to disable this behavior can use the --no-experimental-detect-module flag [1][7][3].
Citations:
- 1: https://nodejs.org/en/blog/release/v20.19.0
- 2: https://nodejs.org/download/release/v20.19.0/docs/api/packages.html
- 3: https://nodejs.org/en/blog/release/v20.19.0/
- 4: https://nodejs.org/en/blog/release/v21.1.0
- 5: https://nodejs.org/en/blog/release/v20.10.0
- 6: v20.19.0 proposal nodejs/node#57349
- 7: https://nodejs.org/download/release/latest-v20.x/docs/api/cli.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== argv log setup and shim callers =="
rg -n -C 5 'writePiArgvShim|argvLog|runPiSubprocess|forward|exitCode' \
test/package/public-cli-cold-matrix.test.ts
echo "== all assertions near cold-matrix scenarios =="
sed -n '1,240p' test/package/public-cli-cold-matrix.test.ts
sed -n '240,430p' test/package/public-cli-cold-matrix.test.ts
sed -n '430,660p' test/package/public-cli-cold-matrix.test.tsRepository: Akagilnc/ak-pi-workflow-roles
Length of output: 29716
🌐 Web query:
site:nodejs.org/download/release/v20.18.0/docs/api/packages.html ambiguous input syntax detection extensionless
💡 Result:
In Node.js v20.18.0, “ambiguous input” means a .js or extensionless file without an explicit module indicator—such as a "type" field in package.json, .mjs/.cjs, or a relevant CLI flag. That version documents syntax detection only behind --experimental-detect-module; if enabled, ES module syntax causes the input to be treated as ESM. (nodejs.org)
Syntax detection became enabled by default in Node.js v20.19.0, allowing extensionless ambiguous scripts containing ESM syntax to run as modules. (nodejs.org)
Citations:
- 1: https://nodejs.org/download/release/v20.18.0/docs/api/packages.html?utm_source=openai
- 2: https://nodejs.org/en/blog/release/v20.19.0?utm_source=openai
Use an explicit module format for the extensionless pi shim.
The project supports Node >=20, but Node 20.0–20.18 does not enable syntax detection by default. On those versions, the shim can fail before writing argvLog; the merger block can then accept the failure as a pre-dispatch error. Use CommonJS syntax or an explicit .mjs implementation.
🤖 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/package/public-cli-cold-matrix.test.ts` around lines 162 - 199, Update
the extensionless pi shim generated in the forward branch of the test helper to
use an explicit module format compatible with Node 20.0–20.18, replacing the ESM
import syntax with CommonJS or another explicit .mjs implementation. Preserve
argvLog writing, child spawning, and exit/signal propagation behavior.
| try { | ||
| const args = JSON.parse(await readFile(argvLog, "utf8")) as string[]; | ||
| if (flagValue(args, "--ak-role") === "merger") { | ||
| assert.equal(args.includes("--ak-merger-input"), true); | ||
| const skill = flagValue(args, "--skill"); | ||
| assert.ok(skill); | ||
| assert.equal( | ||
| skill.includes( | ||
| "resources/methods/resolving-merge-conflicts/SKILL.md", | ||
| ), | ||
| true, | ||
| ); | ||
| assert.equal(skill.includes(".agents/skills"), false); | ||
| } | ||
| } catch (error) { | ||
| // Argv log may still hold the previous role when envelope fails before | ||
| // dispatch; nonzero exit without deferred prose is the contract. | ||
| assert.notEqual(result.code, 0, result.stderr); | ||
| void error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The try/catch makes the merger skill-path assertions non-binding.
assert.equal throws an AssertionError. The catch block at line 630 catches it, then asserts only result.code !== 0. A wrong --skill path, a missing --ak-merger-input, or an ambient .agents/skills path therefore cannot fail this test, because the fallback assertion still passes.
The catch is intended for the readFile failure described in the comment. Scope it to that case. Delete argvLog before the invocation, then branch on whether the file exists instead of catching every error.
🐛 Proposed fix: branch on the argv log instead of catching assertions
+ // Remove the shared log first so a stale role cannot be read here.
+ await rm(argvLog, { force: true });
const result = await runAkRoleBin(
installed.akRoleBin,
[
"merger",
"--project",
project,
"Reconcile the active merge without inventing authority.",
],
{ home, agentDir: piAgentDir, cwd: project, env: shimEnv },
);
assert.equal(result.timedOut, false, result.stderr);
assertNoDeferredSlice("merger", `${result.stdout}\n${result.stderr}`);
// Envelope may fail closed before Pi when no merge is active, or dispatch
// the shell activation path — both are completed public paths.
- try {
- const args = JSON.parse(await readFile(argvLog, "utf8")) as string[];
- if (flagValue(args, "--ak-role") === "merger") {
- assert.equal(args.includes("--ak-merger-input"), true);
- const skill = flagValue(args, "--skill");
- assert.ok(skill);
- assert.equal(
- skill.includes(
- "resources/methods/resolving-merge-conflicts/SKILL.md",
- ),
- true,
- );
- assert.equal(skill.includes(".agents/skills"), false);
- }
- } catch (error) {
- // Argv log may still hold the previous role when envelope fails before
- // dispatch; nonzero exit without deferred prose is the contract.
- assert.notEqual(result.code, 0, result.stderr);
- void error;
- }
+ const dispatched = await access(argvLog).then(
+ () => true,
+ () => false,
+ );
+ if (!dispatched) {
+ // Envelope failed closed before Pi: nonzero exit without deferred prose.
+ assert.notEqual(result.code, 0, result.stderr);
+ } else {
+ const args = JSON.parse(await readFile(argvLog, "utf8")) as string[];
+ assert.equal(flagValue(args, "--ak-role"), "merger");
+ assert.equal(args.includes("--ak-merger-input"), true);
+ const skill = flagValue(args, "--skill");
+ assert.ok(skill);
+ assert.equal(
+ skill.includes("resources/methods/resolving-merge-conflicts/SKILL.md"),
+ true,
+ );
+ assert.equal(skill.includes(".agents/skills"), false);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const args = JSON.parse(await readFile(argvLog, "utf8")) as string[]; | |
| if (flagValue(args, "--ak-role") === "merger") { | |
| assert.equal(args.includes("--ak-merger-input"), true); | |
| const skill = flagValue(args, "--skill"); | |
| assert.ok(skill); | |
| assert.equal( | |
| skill.includes( | |
| "resources/methods/resolving-merge-conflicts/SKILL.md", | |
| ), | |
| true, | |
| ); | |
| assert.equal(skill.includes(".agents/skills"), false); | |
| } | |
| } catch (error) { | |
| // Argv log may still hold the previous role when envelope fails before | |
| // dispatch; nonzero exit without deferred prose is the contract. | |
| assert.notEqual(result.code, 0, result.stderr); | |
| void error; | |
| } | |
| } | |
| // Remove the shared log first so a stale role cannot be read here. | |
| await rm(argvLog, { force: true }); | |
| const result = await runAkRoleBin( | |
| installed.akRoleBin, | |
| [ | |
| "merger", | |
| "--project", | |
| project, | |
| "Reconcile the active merge without inventing authority.", | |
| ], | |
| { home, agentDir: piAgentDir, cwd: project, env: shimEnv }, | |
| ); | |
| assert.equal(result.timedOut, false, result.stderr); | |
| assertNoDeferredSlice("merger", `${result.stdout}\n${result.stderr}`); | |
| // Envelope may fail closed before Pi when no merge is active, or dispatch | |
| // the shell activation path — both are completed public paths. | |
| const dispatched = await access(argvLog).then( | |
| () => true, | |
| () => false, | |
| ); | |
| if (!dispatched) { | |
| // Envelope failed closed before Pi: nonzero exit without deferred prose. | |
| assert.notEqual(result.code, 0, result.stderr); | |
| } else { | |
| const args = JSON.parse(await readFile(argvLog, "utf8")) as string[]; | |
| assert.equal(flagValue(args, "--ak-role"), "merger"); | |
| assert.equal(args.includes("--ak-merger-input"), true); | |
| const skill = flagValue(args, "--skill"); | |
| assert.ok(skill); | |
| assert.equal( | |
| skill.includes("resources/methods/resolving-merge-conflicts/SKILL.md"), | |
| true, | |
| ); | |
| assert.equal(skill.includes(".agents/skills"), 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 `@test/package/public-cli-cold-matrix.test.ts` around lines 616 - 636, Update
the argv-log verification around the JSON parsing block so assertion failures
from the merger checks propagate and fail the test. Delete argvLog before
invoking the command, then branch on whether the log exists: parse and validate
it when present, or otherwise assert the expected nonzero result and
deferred-prose contract for the invocation failure.
2 participants
Summary
Closes #115 technical ACs (cold-install matrix).
Blocked only on owner AC4: approve README public quick-start face (see court escalate).
Does not close #11/#101.
Closes #115
Note
Medium Risk
Changes the public CLI error path for misspelled or future role names (structural reject instead of deferred-slice message) and expands release-candidate install/update coverage; no auth or data-model changes.
Overview
Completes #115 by treating all seven public roles as real
ak-roledispatch paths and removing the “not available in this install slice” deferred stub that previously probed Pi for registered-but-unimplemented roles.The README now documents the full public quick start:
pi update, Fixer/Doctor/Merger invocation examples, Navigator config, and reading Terminal results from stdout instead of scraping Pi sessions. Role docs emphasize package-owned methods from the install rather than home-directory Skills.Tests add a cold-install matrix (
public-cli-cold-matrix.test.ts) that runs the realak-rolebinary through all roles, asserts Pi activation flags and packaged skill paths, verifiespi updaterefreshes CLI and runtime from one private npm copy, and checks the packed artifact ships souls, method trees, and emptypi.extensions. Bin-artifact and unit tests lock in dispatch for every callable role and forbid deferred-slice prose in the shipped bundle. Reviewer lifecycle tests bind code-review from the installed package tree instead of a home Skill fixture.Reviewed by Cursor Bugbot for commit 887bf42. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by Sourcery
Complete the public role matrix for the ak-role CLI and validate cold installs and package updates for all roles and Navigator within a hermetic Pi-managed environment.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests