examples/workflows/README.md (2)

14-19: ⚡ Quick win

Add language specifier to fenced code block.

The code block showing the workflow diagram should have a language specifier (e.g., text or plaintext) for consistency and accessibility.

📝 Suggested fix

-```
+```text
 setup-env → fetch-and-classify → investigate|plan → approve-plan ⏸
 → create-subissues → implement → validate → approve-implementation ⏸
 → create-pr → review-and-classify → [parallel review agents]
 → synthesize → self-fix-and-simplify → linear-finalize

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @examples/workflows/README.md around lines 14 - 19, Add a language specifier
to the fenced code block containing the workflow diagram so it’s
rendered/accessibly labeled (e.g., change the opening triple backticks before
the workflow lines to ```text); update the block that currently contains
"setup-env → fetch-and-classify → investigate|plan → approve-plan ⏸ ..." so the
fenced code block starts with a language token like text or plaintext.

</details>

---

`72-72`: _⚡ Quick win_

**Use descriptive link text instead of "more".**

The link text "more" doesn't convey what users will find at the destination. This impacts accessibility and usability.

<details>
<summary>📝 Suggested fix</summary>

```diff
-Available models: `claude-sonnet-4-6`, `claude-opus-4-7`, `deepseek-3-2`, `minimax-m2-5`, `qwen3-coder-next`, `auto`, and [more](https://github.com/mikeyobrien/pi-provider-kiro#available-models).
+Available models: `claude-sonnet-4-6`, `claude-opus-4-7`, `deepseek-3-2`, `minimax-m2-5`, `qwen3-coder-next`, `auto`, and [see full model list](https://github.com/mikeyobrien/pi-provider-kiro#available-models).
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@examples/workflows/README.md` at line 72, Update the anchor text in the
models list line so it is descriptive instead of "more" — replace
`[more](https://github.com/mikeyobrien/pi-provider-kiro#available-models)` with
a meaningful label such as `[available
models](https://github.com/mikeyobrien/pi-provider-kiro#available-models)` or
`[provider available
models](https://github.com/mikeyobrien/pi-provider-kiro#available-models)` in
the README line that lists Available models (`claude-sonnet-4-6`,
`claude-opus-4-7`, `deepseek-3-2`, `minimax-m2-5`, `qwen3-coder-next`, `auto`)
so the link clearly describes its destination for accessibility and usability.
```

</details>

</blockquote></details>
<details>
<summary>packages/providers/src/community/pi/provider.test.ts (1)</summary><blockquote>

`393-407`: _⚡ Quick win_

**Consider adding a test that verifies `session.setModel()` is called for extension-provided models.**

The tests verify the error path when `find()` returns undefined twice, but there's no test covering the happy path where `find()` returns undefined initially (phase 1), then returns a model after `bindExtensions()` (phase 2), triggering `session.setModel()`. This is the primary success path for extension-provided models like `kiro/minimax-m2-5`.

<details>
<summary>💡 Example test structure</summary>

```typescript
test('deferred model resolution calls session.setModel after bindExtensions', async () => {
  process.env.GEMINI_API_KEY = 'sk-test';
  // First find() returns undefined (not in static catalog)
  mockModelRegistryFind.mockImplementationOnce(() => undefined);
  // Second find() after bindExtensions returns the model
  mockModelRegistryFind.mockImplementationOnce((provider, modelId) => ({
    id: modelId,
    provider,
    name: `${provider}/${modelId}`,
  }));
  resetScript(scriptedAgentEnd());

  const { error } = await consume(
    new PiProvider().sendQuery('hi', '/tmp', undefined, {
      model: 'kiro/claude-sonnet-4-6',
    })
  );

  expect(error).toBeUndefined();
  expect(mockSetModel).toHaveBeenCalledTimes(1);
  expect(mockSetModel).toHaveBeenCalledWith(
    expect.objectContaining({ provider: 'kiro' })
  );
});
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@packages/providers/src/community/pi/provider.test.ts` around lines 393 - 407,
Add a happy-path unit test that verifies deferred model resolution triggers
session.setModel: when testing PiProvider.sendQuery, mockModelRegistryFind to
first return undefined (static catalog miss) and then return a model object on
the subsequent call (after bindExtensions), call
resetScript(scriptedAgentEnd()), invoke new PiProvider().sendQuery(...) with an
extension-provided model id (e.g., "kiro/..."), then assert no error and that
mockSetModel (or session.setModel) was called exactly once with an object whose
provider equals the extension provider (e.g., 'kiro'); use the existing mocks
(mockModelRegistryFind, mockSetModel) and ensure the second
mockModelRegistryFind implementation returns an object containing id, provider,
and name so session.setModel is exercised.
```

</details>

</blockquote></details>
<details>
<summary>packages/providers/src/community/pi/provider.ts (1)</summary><blockquote>

`262-286`: _💤 Low value_

**Step numbering inconsistency: "4" is reused.**

The comment at line 262 starts with "4. Resolve credentials" but line 326 also starts with "4. Translate Archon nodeConfig". This makes the step sequence confusing. Steps 4e, 4f, and 4g reference a different "step 4" than the credential resolution block.

Consider renumbering to use distinct step numbers (e.g., credentials as step 3, nodeConfig translation as step 4) or converting to descriptive section headers instead of numbered steps.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@packages/providers/src/community/pi/provider.ts` around lines 262 - 286, The
comment block labeled "4. Resolve credentials" duplicates a "step 4" used later
for "4. Translate Archon nodeConfig", causing numbering confusion; update the
comment headings to unique identifiers (e.g., renumber credentials to "3.
Resolve credentials" or replace numeric steps with descriptive headers) and
adjust any sub-step labels (4e/4f/4g) to match the new scheme so references
remain consistent; ensure you update the surrounding comments that reference
parsed.provider, PI_PROVIDER_ENV_VARS, envOverride, and
authStorage.setRuntimeApiKey to reflect the new step/header naming.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @examples/workflows/README.md:

• Around line 14-19: Add a language specifier to the fenced code block
  containing the workflow diagram so it’s rendered/accessibly labeled (e.g.,
  change the opening triple backticks before the workflow lines to ```text);
  update the block that currently contains "setup-env → fetch-and-classify →
  investigate|plan → approve-plan ⏸ ..." so the fenced code block starts with a
  language token like text or plaintext.
• Line 72: Update the anchor text in the models list line so it is descriptive
  instead of "more" — replace
  [more](https://github.com/mikeyobrien/pi-provider-kiro#available-models) with
  a meaningful label such as [available models](https://github.com/mikeyobrien/pi-provider-kiro#available-models) or
  [provider available models](https://github.com/mikeyobrien/pi-provider-kiro#available-models) in
  the README line that lists Available models (claude-sonnet-4-6,
  claude-opus-4-7, deepseek-3-2, minimax-m2-5, qwen3-coder-next, auto)
  so the link clearly describes its destination for accessibility and usability.

In @packages/providers/src/community/pi/provider.test.ts:

• Around line 393-407: Add a happy-path unit test that verifies deferred model
  resolution triggers session.setModel: when testing PiProvider.sendQuery,
  mockModelRegistryFind to first return undefined (static catalog miss) and then
  return a model object on the subsequent call (after bindExtensions), call
  resetScript(scriptedAgentEnd()), invoke new PiProvider().sendQuery(...) with an
  extension-provided model id (e.g., "kiro/..."), then assert no error and that
  mockSetModel (or session.setModel) was called exactly once with an object whose
  provider equals the extension provider (e.g., 'kiro'); use the existing mocks
  (mockModelRegistryFind, mockSetModel) and ensure the second
  mockModelRegistryFind implementation returns an object containing id, provider,
  and name so session.setModel is exercised.

In @packages/providers/src/community/pi/provider.ts:

• Around line 262-286: The comment block labeled "4. Resolve credentials"
  duplicates a "step 4" used later for "4. Translate Archon nodeConfig", causing
  numbering confusion; update the comment headings to unique identifiers (e.g.,
  renumber credentials to "3. Resolve credentials" or replace numeric steps with
  descriptive headers) and adjust any sub-step labels (4e/4f/4g) to match the new
  scheme so references remain consistent; ensure you update the surrounding
  comments that reference parsed.provider, PI_PROVIDER_ENV_VARS, envOverride, and
  authStorage.setRuntimeApiKey to reflect the new step/header naming.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: defaults

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `8258c687-d2d9-4b03-8c25-a37e726cfdcb`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 8295ece76311a55264ee4c8e9af2c8395b626e50 and 1fbdee30e9835079ae9d2c3dd59c9de1a12ab16a.

</details>

<details>
<summary>📒 Files selected for processing (4)</summary>

* `examples/workflows/README.md`
* `examples/workflows/archon-test-pi.yaml`
* `packages/providers/src/community/pi/provider.test.ts`
* `packages/providers/src/community/pi/provider.ts`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

packages/providers/src/community/pi/provider.ts (1)

475-476: ⚡ Quick win

Update stale mutability comment for ModelRegistry.create().

These lines say create() is immutable and inMemory() is required, but this file now intentionally relies on ModelRegistry.create() being mutable (also documented earlier around Line 207-Line 212). Please align this comment to prevent future regressions.

Proposed comment fix

-    //     ModelRegistry.inMemory() is required (step 2) — create() returns
-    //     an immutable registry that registerProvider() can't modify.
+    //     This works with ModelRegistry.create() (step 2): extension
+    //     session_start hooks can call registerProvider() and mutate the
+    //     same registry instance used by this provider.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/providers/src/community/pi/provider.ts` around lines 475 - 476, The
comment claiming ModelRegistry.create() returns an immutable registry (and that
ModelRegistry.inMemory() is required) is stale; update the comment to state that
ModelRegistry.create() produces a mutable registry that registerProvider() can
modify (consistent with the earlier ModelRegistry.create() documentation) and
remove or clarify the note about inMemory() being required so future readers
won't revert this intentional behavior; locate the comment near the existing
references to ModelRegistry.create(), ModelRegistry.inMemory(), and
registerProvider() and rewrite it to accurately describe mutability and intended
usage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes