Repository navigation
feat: workspace review for other clients — your requests in one view, withdraw and remove from the TUI and serve, decisions for plugins - #1716
Conversation
… withdraw and remove from the TUI and serve, decisions at plugin session start
- `GET /altimate/workspace/requests`: the person's own requests on the linked workspace in one list — rule proposals
(local state) and skill and knowledge requests (server, one captured credential checked against the signed-in
account) — with status, reviewer, reason, version and the actions allowed (withdraw a waiting one, remove an approved
one). `review_status` says why skills and knowledge are missing when they are.
- `POST /altimate/workspace/requests/{id}/withdraw|remove`: the author withdraws or removes a skill or knowledge
request (`ReviewApi.withdraw`/`retire`); refusals keep the server's code and status; clients are told to refresh.
- The rules summary gains `decisions`: rule reviews and skill and knowledge decisions in one list, newest first, each
remembered by `decisionKey` (`kind:public_id:status`); `notices` and `review_notices` stay for released clients.
- `POST /altimate/skill/publish` refusals carry a stable `code` (`review_disabled`, `too_many_requests`, …).
- TUI `/rules` lists your skill and knowledge requests under your proposals; picking one offers Withdraw or Remove from
the workspace, after a confirm.
- Host (Claude Code / Codex) session start polls the review state once (3s at most, then what was kept) and tells the
session of skill and knowledge decisions not reported yet (`host_review_seen`), and how many requests still wait.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ecisions - `RulesService.pollReviewDecisions`: one poll that publishes each new decision as `rule.proposal.reviewed` (with its kind) and asks clients to replay when something was recorded; used by the refresh timer and now by `POST /altimate/rules/sync`, so a person who presses Sync now sees an approval or removal at once instead of within the next 5 minutes. Found in the extension end-to-end run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t show decisions themselves
- `altimate-code workspace review decisions --json [--unseen] [--mark-seen] [--host <name>] [--directory <dir>]`:
one bounded poll (3s, then what was kept), then the person's skill and knowledge decisions newest first with the
waiting count. `--unseen --host H` gives only those not given to H yet (last 14 days); `--mark-seen` remembers them
for H (`host_seen` in the review state, capped). Always JSON and exit 0; `{ok:false,status}` says why when there is
nothing to read (signed_out, not_linked, unsupported, …). For the Claude Code plugin's toast.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rded elsewhere reach the TUI, bounded /rules, printable text
- `workspace review decisions`: a linked person with nothing kept yet answers `{ok:true,decisions:[]}`; a poll that did
not finish in time answers `account_unconfirmed` (asked again), never `not_linked` (which stops the Claude toast for
the session).
- `pollReviewDecisions` asks clients to replay when what is kept changed since this process last looked, so a decision
first recorded by a plugin's CLI process still reaches a running TUI or extension.
- TUI `/rules` waits at most 5s for your skill and knowledge requests, then opens without them and says so.
- `review_error` on `/altimate/workspace/requests` and the names, reasons and people in
`workspace review status --json` are made printable.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pre-merge checks:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check |  |
This assessment has not completed. The check will be retried on the next pre-merge check run. | |
| Description check | |
This assessment has not completed. The check will be retried on the next pre-merge check run. |
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
23 issues found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/test/altimate/workspace/skill-publish.test.ts">
<violation number="1" location="packages/opencode/test/altimate/workspace/skill-publish.test.ts:1431">
P3: This test does not cover most stable refusal codes despite claiming to test each deliberate refusal. Add assertions for the remaining typed errors so changes to their client-facing codes cannot regress unnoticed.</violation>
</file>
<file name="packages/opencode/test/server/altimate-rules-routes.test.ts">
<violation number="1" location="packages/opencode/test/server/altimate-rules-routes.test.ts:130">
P3: This test never exercises the rule half of this route: the fixture has no local `RulesStore` state, so `requests: []` still passes if `RulesSummary.rows` is omitted. Seed one local sent rule and assert its mapped request.</violation>
</file>
<file name="packages/opencode/test/altimate/workspace/review.test.ts">
<violation number="1" location="packages/opencode/test/altimate/workspace/review.test.ts:610">
P2: This test does not exercise the rule/skill merge or newest-first ordering it claims to cover. Add a rule decision and at least two differently dated decisions, then assert their merged order.</violation>
<violation number="2" location="packages/opencode/test/altimate/workspace/review.test.ts:629">
P2: The “polls once” contract is not enforced because the test never counts network calls. Count `/reviews` requests before and after `reviewForHost` and assert the difference is one.</violation>
<violation number="3" location="packages/opencode/test/altimate/workspace/review.test.ts:666">
P2: This case does not verify either the signed-out reason or the separate not-linked result promised by its name. Assert `status: "signed_out"` and add an unlinked-project case expecting `not_linked`.</violation>
</file>
<file name="packages/opencode/src/altimate/rules/store.ts">
<violation number="1" location="packages/opencode/src/altimate/rules/store.ts:242">
P2: Resetting or relinking a project drops `host_review_seen`, so the next host session repeats recent skill and knowledge decisions that were already reported. Preserve this cursor in `cleared()` alongside `host_notices_seen`.</violation>
</file>
<file name="packages/opencode/src/server/server.ts">
<violation number="1" location="packages/opencode/src/server/server.ts:1398">
P2: A thrown rules sync aborts before `pollReviewDecisions`, so Sync now skips skill and knowledge decision polling on lock or state failures. Run the decision poll independently, such as in a `finally` block.</violation>
<violation number="2" location="packages/opencode/src/server/server.ts:1427">
P2: This new response returns rule text, reasons, and reviewer names without `printable`, allowing control or bidi characters from synced state to reach clients. Sanitize these fields before serializing the request list.</violation>
</file>
<file name="packages/opencode/src/plugin/tui/altimate/rules.tsx">
<violation number="1" location="packages/opencode/src/plugin/tui/altimate/rules.tsx:530">
P3: Clear or unref the five-second timeout when the request wins the race; otherwise a fast `/rules` invocation keeps a live timer until the deadline and can delay clean shutdown after the TUI closes.</violation>
<violation number="2" location="packages/opencode/src/plugin/tui/altimate/rules.tsx:532">
P2: The menu silently treats `review_status` failures or unavailable accounts as “no skill and knowledge requests.” Show an unavailable/error row (including the printable server error where present) whenever the response is not `ok` for review, rather than only handling the timeout case.</violation>
<violation number="3" location="packages/opencode/src/plugin/tui/altimate/rules.tsx:671">
P2: `showRequestActions` executes the destructive `withdraw`/`remove` request immediately after selection, despite promising confirmation. Add a confirmation step before posting either action.</violation>
</file>
<file name="packages/opencode/src/altimate/rules/summary.ts">
<violation number="1" location="packages/opencode/src/altimate/rules/summary.ts:101">
P2: `decisions` drops all but the latest ten rule notices, although this field promises every decision and the store retains 50. Clients offline for more than ten rule reviews cannot replay the older decisions; use the retained notice list without the extra slice.</violation>
</file>
<file name="packages/opencode/src/altimate/workspace/skill-publish.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/skill-publish.ts:1095">
P2: `publishErrorCode` omits seven deliberate refusal types that `explainPublishError` exposes, so the serve endpoint returns 422 responses without a stable code for built-in/managed, invalid-bundle, and configuration failures. Add codes for every deliberate refusal that this endpoint can return so clients do not have to parse the message.</violation>
</file>
<file name="packages/opencode/src/altimate/workspace/review-notices.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/review-notices.ts:87">
P2: The persisted host-seen map is unbounded across host names: each distinct `--host` value adds another entry even though `MAX_HOST_SEEN` caps only one entry. Bound the number of host entries or restrict hosts to canonical client identifiers.</violation>
<violation number="2" location="packages/opencode/src/altimate/workspace/review-notices.ts:383">
P3: A slow first poll is reported as `account_unconfirmed` when no cached state exists, mislabeling a timeout as an authentication problem. Return a distinct timeout status so clients can retry without presenting the wrong failure reason.</violation>
<violation number="3" location="packages/opencode/src/altimate/workspace/review-notices.ts:391">
P2: `hostDecisions` reverses notices that `poll` appends in newest-first API order, so a poll recording multiple decisions returns the oldest decision first. Sort by decision time descending, or append notices chronologically before reversing.</violation>
<violation number="4" location="packages/opencode/src/altimate/workspace/review-notices.ts:394">
P2: Concurrent `--unseen --mark-seen` invocations can both return the same decision for one host. Select and return the newly unseen keys inside the same locked read-modify transaction, rather than filtering before `locked`.</violation>
</file>
<file name="packages/opencode/src/cli/cmd/workspace.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/workspace.ts:387">
P2: The `...item` spread still emits `Contribution.description` unchanged in `workspace review status --json`. Sanitize this server-provided text before emitting the JSON so control and bidi characters cannot reach consumers.</violation>
</file>
<file name="packages/opencode/src/altimate/rules/host.ts">
<violation number="1" location="packages/opencode/src/altimate/rules/host.ts:187">
P2: The new review poll remains behind the `rulesEnabled` early return, so disabling team-rule learning prevents host sessions from fetching or reporting skill and knowledge review decisions. Run the review path independently of the learning gate, while retaining the workspace/link checks for the data it can access.</violation>
</file>
<file name="packages/opencode/src/altimate/rules/service.ts">
<violation number="1" location="packages/opencode/src/altimate/rules/service.ts:113">
P3: `lastKept` grows for every directory ever polled and is never cleaned up on instance disposal, so a long-lived server accumulates workspace paths indefinitely. Remove the directory entry when its instance is disposed or bound the cache with eviction.</violation>
<violation number="2" location="packages/opencode/src/altimate/rules/service.ts:138">
P2: `pollReviewDecisions` can publish `Changed` after its owning instance is disposed because the helper has no post-poll disposal guard. Preserve the old `disposed` check by passing an instance-liveness predicate into the helper before publishing.</violation>
</file>
<file name="packages/opencode/src/altimate/workspace/review-requests.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/review-requests.ts:121">
P1: The binding and credential are not captured atomically: a credential switch during `resolveBindingOutcome()` can pair one account's workspace ID with another account's `actAs`. Resolve the binding using the captured credential scope, or otherwise prevent and reject scope changes during resolution before listing or mutating requests.</violation>
<violation number="2" location="packages/opencode/src/altimate/workspace/review-requests.ts:122">
P2: This maps an unconfirmed workspace link to `not_linked`, so an outage can make clients show that the project is unlinked and hide valid requests. Preserve the unresolved state or return a failed/link-unconfirmed result instead of treating it as an absent link.
(Based on your team's feedback about preserving unknown binding state.) [a017d85a-eb29-4761-92ae-4c81bcb8d031]</violation>
</file>
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic
| ]) | ||
| const actAs = await WorkspaceApi.captureCredentials() | ||
| if (!actAs) return { ready: false, status: "signed_out" } | ||
| const outcome = await resolveBindingOutcome(directory).catch(() => ({ status: "unknown" as const })) |
There was a problem hiding this comment.
P1: The binding and credential are not captured atomically: a credential switch during resolveBindingOutcome() can pair one account's workspace ID with another account's actAs. Resolve the binding using the captured credential scope, or otherwise prevent and reject scope changes during resolution before listing or mutating requests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/workspace/review-requests.ts, line 121:
<comment>The binding and credential are not captured atomically: a credential switch during `resolveBindingOutcome()` can pair one account's workspace ID with another account's `actAs`. Resolve the binding using the captured credential scope, or otherwise prevent and reject scope changes during resolution before listing or mutating requests.</comment>
<file context>
@@ -0,0 +1,127 @@
+ ])
+ const actAs = await WorkspaceApi.captureCredentials()
+ if (!actAs) return { ready: false, status: "signed_out" }
+ const outcome = await resolveBindingOutcome(directory).catch(() => ({ status: "unknown" as const }))
+ if (outcome.status !== "bound") return { ready: false, status: "not_linked" }
+ if ((await accountDigest()) !== credentialDigest(actAs.url, actAs.instance, actAs.apiKey)) return { ready: false, status: "account_changed" }
</file context>
|
|
||
| test("without a link or a sign-in it says why", async () => { | ||
| configured = false | ||
| expect(await ReviewNotices.hostDecisions(project, { host: "claude-ui", unseen: true, waitMs: 500 })).toMatchObject({ ok: false }) |
There was a problem hiding this comment.
P2: This case does not verify either the signed-out reason or the separate not-linked result promised by its name. Assert status: "signed_out" and add an unlinked-project case expecting not_linked.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/altimate/workspace/review.test.ts, line 666:
<comment>This case does not verify either the signed-out reason or the separate not-linked result promised by its name. Assert `status: "signed_out"` and add an unlinked-project case expecting `not_linked`.</comment>
<file context>
@@ -544,3 +544,125 @@ describe("workspace review status", () => {
+
+ test("without a link or a sign-in it says why", async () => {
+ configured = false
+ expect(await ReviewNotices.hostDecisions(project, { host: "claude-ui", unseen: true, waitMs: 500 })).toMatchObject({ ok: false })
+ })
+})
</file context>
| contribution({ status: "active", reviewed_by_name: "Olive", reviewed_at: new Date().toISOString() }), | ||
| contribution({ public_id: "c-2", status: "pending" }), | ||
| ]) | ||
| const first = await RulesHost.reviewForHost(project, new Set(), 3000) |
There was a problem hiding this comment.
P2: The “polls once” contract is not enforced because the test never counts network calls. Count /reviews requests before and after reviewForHost and assert the difference is one.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/altimate/workspace/review.test.ts, line 629:
<comment>The “polls once” contract is not enforced because the test never counts network calls. Count `/reviews` requests before and after `reviewForHost` and assert the difference is one.</comment>
<file context>
@@ -544,3 +544,125 @@ describe("workspace review status", () => {
+ contribution({ status: "active", reviewed_by_name: "Olive", reviewed_at: new Date().toISOString() }),
+ contribution({ public_id: "c-2", status: "pending" }),
+ ])
+ const first = await RulesHost.reviewForHost(project, new Set(), 3000)
+ expect(first.decisions.map((d) => [d.public_id, d.status, d.reviewer])).toEqual([["c-1", "active", "Olive"]])
+ expect(first.waiting).toBe(1)
</file context>
| const first = await RulesHost.reviewForHost(project, new Set(), 3000) | |
| const reviewCallsBefore = requests.filter((r) => r.url.includes("/reviews")).length | |
| const first = await RulesHost.reviewForHost(project, new Set(), 3000) | |
| expect(requests.filter((r) => r.url.includes("/reviews")).length - reviewCallsBefore).toBe(1) |
| }) | ||
|
|
||
| test("the summary's decisions put rule and skill decisions in one list, newest first", async () => { | ||
| mine = () => page([contribution({ status: "pending" })]) |
There was a problem hiding this comment.
P2: This test does not exercise the rule/skill merge or newest-first ordering it claims to cover. Add a rule decision and at least two differently dated decisions, then assert their merged order.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/altimate/workspace/review.test.ts, line 610:
<comment>This test does not exercise the rule/skill merge or newest-first ordering it claims to cover. Add a rule decision and at least two differently dated decisions, then assert their merged order.</comment>
<file context>
@@ -544,3 +544,125 @@ describe("workspace review status", () => {
+ })
+
+ test("the summary's decisions put rule and skill decisions in one list, newest first", async () => {
+ mine = () => page([contribution({ status: "pending" })])
+ await poll()
+ mine = () => page([contribution({ status: "rejected", reviewed_by_name: "Olive", review_reason: "No", reviewed_at: "2026-11-02T00:00:00Z" })])
</file context>
| host_notices_seen: z.array(z.string()).default([]), | ||
| /** Skill and knowledge decisions already reported at a host session start (`kind:public_id:status`). Apart from | ||
| * `host_notices_seen`, whose trimming knows only rule notices. */ | ||
| host_review_seen: z.array(z.string()).default([]), |
There was a problem hiding this comment.
P2: Resetting or relinking a project drops host_review_seen, so the next host session repeats recent skill and knowledge decisions that were already reported. Preserve this cursor in cleared() alongside host_notices_seen.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/rules/store.ts, line 242:
<comment>Resetting or relinking a project drops `host_review_seen`, so the next host session repeats recent skill and knowledge decisions that were already reported. Preserve this cursor in `cleared()` alongside `host_notices_seen`.</comment>
<file context>
@@ -236,6 +237,9 @@ export const State = z.object({
host_notices_seen: z.array(z.string()).default([]),
+ /** Skill and knowledge decisions already reported at a host session start (`kind:public_id:status`). Apart from
+ * `host_notices_seen`, whose trimming knows only rule notices. */
+ host_review_seen: z.array(z.string()).default([]),
/** Host sessions whose check stopped with work left (the round limit): the next check with nothing new takes them first. */
host_pending: z.array(z.string()).default([]),
</file context>
| describe("refusal codes for clients", () => { | ||
| test("each deliberate refusal has a stable code; anything else has none", async () => { | ||
| const mod = await import("../../../src/altimate/workspace/skill-publish") | ||
| expect(mod.publishErrorCode(new mod.ReviewDisabledError("Growth", "Olive"))).toBe("review_disabled") |
There was a problem hiding this comment.
P3: This test does not cover most stable refusal codes despite claiming to test each deliberate refusal. Add assertions for the remaining typed errors so changes to their client-facing codes cannot regress unnoticed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/altimate/workspace/skill-publish.test.ts, line 1431:
<comment>This test does not cover most stable refusal codes despite claiming to test each deliberate refusal. Add assertions for the remaining typed errors so changes to their client-facing codes cannot regress unnoticed.</comment>
<file context>
@@ -1424,3 +1424,13 @@ describe("workspace review", () => {
+describe("refusal codes for clients", () => {
+ test("each deliberate refusal has a stable code; anything else has none", async () => {
+ const mod = await import("../../../src/altimate/workspace/skill-publish")
+ expect(mod.publishErrorCode(new mod.ReviewDisabledError("Growth", "Olive"))).toBe("review_disabled")
+ expect(mod.publishErrorCode(new mod.TooManyReviewRequestsError("Growth"))).toBe("too_many_requests")
+ expect(mod.publishErrorCode(new mod.NotLinkedError())).toBe("not_linked")
</file context>
| const response = await Server.Default().request("/altimate/workspace/requests", { headers: { "x-opencode-directory": dir.path } }) | ||
| expect(response.status).toBe(200) | ||
| const body = await response.json() | ||
| expect(body).toMatchObject({ ok: true, requests: [] }) |
There was a problem hiding this comment.
P3: This test never exercises the rule half of this route: the fixture has no local RulesStore state, so requests: [] still passes if RulesSummary.rows is omitted. Seed one local sent rule and assert its mapped request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/test/server/altimate-rules-routes.test.ts, line 130:
<comment>This test never exercises the rule half of this route: the fixture has no local `RulesStore` state, so `requests: []` still passes if `RulesSummary.rows` is omitted. Seed one local sent rule and assert its mapped request.</comment>
<file context>
@@ -98,3 +98,53 @@ describe("rule proposal routes", () => {
+ const response = await Server.Default().request("/altimate/workspace/requests", { headers: { "x-opencode-directory": dir.path } })
+ expect(response.status).toBe(200)
+ const body = await response.json()
+ expect(body).toMatchObject({ ok: true, requests: [] })
+ expect(["signed_out", "not_linked"]).toContain(body.review_status)
+ })
</file context>
| // Bounded: the list asks the server (the link, every page of requests); a slow server must not hold the menu. | ||
| const listed = await Promise.race([ | ||
| get<RequestsResponse>(api, "/altimate/workspace/requests"), | ||
| new Promise<"slow">((resolve) => setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS)), |
There was a problem hiding this comment.
P3: Clear or unref the five-second timeout when the request wins the race; otherwise a fast /rules invocation keeps a live timer until the deadline and can delay clean shutdown after the TUI closes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/plugin/tui/altimate/rules.tsx, line 530:
<comment>Clear or unref the five-second timeout when the request wins the race; otherwise a fast `/rules` invocation keeps a live timer until the deadline and can delay clean shutdown after the TUI closes.</comment>
<file context>
@@ -523,6 +523,14 @@ async function showMenu(api: TuiPluginApi) {
+ // Bounded: the list asks the server (the link, every page of requests); a slow server must not hold the menu.
+ const listed = await Promise.race([
+ get<RequestsResponse>(api, "/altimate/workspace/requests"),
+ new Promise<"slow">((resolve) => setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS)),
+ ])
+ const requests = (listed === "slow" ? [] : (listed?.requests ?? [])).filter((r) => r.kind !== "rule")
</file context>
| * (with its `kind`) to clients listening, and `altimate.rules.changed` asks them to replay when anything was recorded | ||
| * (or `announceChange`). For the refresh timer and "Sync now". Throws only what the poll does not swallow. */ | ||
| /** The newest kept decision per directory, as this process last saw it. */ | ||
| const lastKept = new Map<string, string>() |
There was a problem hiding this comment.
P3: lastKept grows for every directory ever polled and is never cleaned up on instance disposal, so a long-lived server accumulates workspace paths indefinitely. Remove the directory entry when its instance is disposed or bound the cache with eviction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/rules/service.ts, line 113:
<comment>`lastKept` grows for every directory ever polled and is never cleaned up on instance disposal, so a long-lived server accumulates workspace paths indefinitely. Remove the directory entry when its instance is disposed or bound the cache with eviction.</comment>
<file context>
@@ -106,6 +106,40 @@ function triggerDeps(ctx: InstanceContext, model?: string): RuleTrigger.TriggerD
+ * (with its `kind`) to clients listening, and `altimate.rules.changed` asks them to replay when anything was recorded
+ * (or `announceChange`). For the refresh timer and "Sync now". Throws only what the poll does not swallow. */
+/** The newest kept decision per directory, as this process last saw it. */
+const lastKept = new Map<string, string>()
+
+export async function pollReviewDecisions(directory: string, opts: { announceChange?: boolean } = {}) {
</file context>
| // A poll that finished with nothing kept: linked, with nothing decided yet (no state file is written until there | ||
| // is something to keep). A poll that did not finish in time is not an answer: asked again later. | ||
| if (polled?.status === "done") return { ok: true, decisions: [], waiting: 0 } | ||
| return { ok: false, status: polled ? polled.status : "account_unconfirmed" } |
There was a problem hiding this comment.
P3: A slow first poll is reported as account_unconfirmed when no cached state exists, mislabeling a timeout as an authentication problem. Return a distinct timeout status so clients can retry without presenting the wrong failure reason.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/opencode/src/altimate/workspace/review-notices.ts, line 383:
<comment>A slow first poll is reported as `account_unconfirmed` when no cached state exists, mislabeling a timeout as an authentication problem. Return a distinct timeout status so clients can retry without presenting the wrong failure reason.</comment>
<file context>
@@ -312,35 +315,91 @@ async function pollUnsafe(directory: string, deps: PollDeps): Promise<PollResult
+ // A poll that finished with nothing kept: linked, with nothing decided yet (no state file is written until there
+ // is something to keep). A poll that did not finish in time is not an answer: asked again later.
+ if (polled?.status === "done") return { ok: true, decisions: [], waiting: 0 }
+ return { ok: false, status: polled ? polled.status : "account_unconfirmed" }
+ }
+ if (polled && polled.status === "unsupported") return { ok: false, status: "unsupported" }
</file context>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const now = Date.now() | ||
| const decisions = [...state.notices] | ||
| .reverse() | ||
| .filter((n) => !opts.unseen || !host || (!seen.has(key(n)) && !(now - Date.parse(n.at) > HOST_DECISION_MAX_AGE_MS))) |
There was a problem hiding this comment.
WARNING: --unseen silently returns everything when --host is omitted
The filter is !opts.unseen || !host || (...): with unseen: true and no host the whole condition short-circuits and every kept notice is returned, including decisions already given to another host and ones older than HOST_DECISION_MAX_AGE_MS. The CLI exposes --unseen and --host independently, so workspace review decisions --unseen does not do what its help says, and --mark-seen without --host is a silent no-op (line 393). Require host for both (e.g. .implies) or apply the age cap even without a host.
| * (with its `kind`) to clients listening, and `altimate.rules.changed` asks them to replay when anything was recorded | ||
| * (or `announceChange`). For the refresh timer and "Sync now". Throws only what the poll does not swallow. */ | ||
| /** The newest kept decision per directory, as this process last saw it. */ | ||
| const lastKept = new Map<string, string>() |
There was a problem hiding this comment.
WARNING: process-global lastKept is never cleaned up and is keyed by directory only
Entries are added but never deleted — the instance disposer at the end of start() does not touch this map — so a long-lived serve keeps one path/key per project directory for the process lifetime, and tests that reuse a directory inherit a stale baseline. The key also ignores the resolved review scope (apiUrl/instance/account/datamateId): after an account switch, pin change or relink the map still holds the previous scope's newest key, so a genuinely new decision with the same kind:public_id:status is suppressed. Key by the resolved scope (or hold it per InstanceState) and delete on dispose.
| if (result.status === "done") { | ||
| const newest = (await ReviewNotices.storedState(directory))?.notices.at(-1) | ||
| const key = newest ? `${newest.kind}:${newest.public_id}:${newest.status}` : "" | ||
| keptChanged = lastKept.has(directory) && lastKept.get(directory) !== key |
There was a problem hiding this comment.
WARNING: the first poll can never report keptChanged
keptChanged requires lastKept.has(directory), so the first done poll of a process always seeds the baseline without announcing. The refresh timer compensates with announceChange: !reviewsPolled, but POST /altimate/rules/sync calls pollReviewDecisions(Instance.directory) with no option (server.ts:1400): if another process recorded the decision before this process's first poll, this poll records nothing and every later poll sees the same key, so the "a decision recorded elsewhere still makes clients replay" case this change targets is missed. Treat an absent baseline as changed on the first done poll. (storedState also returns undefined for transient read failures, mapping to key = "", which can turn a failed read into a spurious announcement.)
| notices: state.notices.slice(-10).reverse(), | ||
| review_notices: reviewNotices, | ||
| decisions: [ | ||
| ...state.notices.slice(-10).map((n): Decision => ({ kind: "rule", ...n })), |
There was a problem hiding this comment.
SUGGESTION: decisions is documented as "every decision", but the rule half is capped at 10
ReviewNotices keeps up to MAX_NOTICES (50) decisions and review_notices replays all of them, while the rule half is state.notices.slice(-10), so rule reviews 11–50 are silently dropped from the combined list clients use to show each decision once. The same slice(-10) is also computed twice (line 98); use one variable and the store's kept window for both halves.
| host_notices_seen: z.array(z.string()).default([]), | ||
| /** Skill and knowledge decisions already reported at a host session start (`kind:public_id:status`). Apart from | ||
| * `host_notices_seen`, whose trimming knows only rule notices. */ | ||
| host_review_seen: z.array(z.string()).default([]), |
There was a problem hiding this comment.
WARNING: host_review_seen is lost on reset({ keepOutbox: true })
cleared() preserves host_notices_seen and the other host fields for a same-scope link drop/return, but the new host_review_seen is not listed there, so ...structuredClone(EMPTY) resets it to []. The per-scope review state still holds up to 50 notices, so after a sign-out/sign-in or a dropped-and-returned link the next host session start re-reports every prior skill/knowledge decision within NOTICE_MAX_AGE_MS, defeating the once-per-host guarantee this field exists for. Add host_review_seen: state.host_review_seen to the preserved fields in cleared().
| lines.push(`Decisions on the skills and knowledge bases the person sent to ${ws} for review, since their last session (tell them):`, ...start.review_decisions.map(line)) | ||
| if (start.review_waiting) { | ||
| const one = start.review_waiting === 1 | ||
| lines.push(`${one ? "One" : start.review_waiting} of the person's skill or knowledge requests ${one ? "is" : "are"} waiting for review in ${ws}; ${command} lists ${one ? "it" : "them"}.`) |
There was a problem hiding this comment.
SUGGESTION: ${command} does not list skill or knowledge requests
context() defaults command to /altimate-code:rules, which in this repo is altimate-code learn proposals (docs/configure/learn.md:200,222) and lists only stored rule proposals and rule rows — not the person's skill/knowledge requests. The line therefore tells the person (and the agent) to run a command that will show nothing about the waiting request. Point at altimate-code workspace review status (or /rules in the TUI) here.
| get<RequestsResponse>(api, "/altimate/workspace/requests"), | ||
| new Promise<"slow">((resolve) => setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS)), | ||
| ]) | ||
| const requests = (listed === "slow" ? [] : (listed?.requests ?? [])).filter((r) => r.kind !== "rule") |
There was a problem hiding this comment.
SUGGESTION: a failed or partial listing renders as "no requests"
The route reports why skills/knowledge are missing (review_status: signed_out, not_linked, account_changed, unsupported, failed) and whether the list was cut (review_complete: false), but this line reads only requests, and get() swallows transport/HTTP errors to undefined. A signed-out, failed or truncated listing is therefore shown exactly like "you have no requests". Surface review_status/review_complete: false as a row or in the category label.
| if (option.value.startsWith("request:")) { | ||
| const request = requests.find((r) => `request:${r.public_id}` === option.value) | ||
| if (request?.actions.length) return showRequestActions(api, request) | ||
| return |
There was a problem hiding this comment.
SUGGESTION: selecting a request with no actions does nothing
Rows for rejected, superseded, withdrawn and retired requests are in requests (only kind === "rule" is filtered) but fromContribution gives them actions: [], so this branch returns without opening anything, closing the menu, or telling the user; DialogSelect does not close itself, so Enter on those rows is a silent no-op. Skip rows without actions from the list, or open a status-only dialog.
| workspace_name: printable(binding.datamateName), | ||
| ...listed, | ||
| // Server text reaches terminals and agent context: made printable here, as the text form is. | ||
| items: listed.items.map((item) => ({ |
There was a problem hiding this comment.
SUGGESTION: the printable pass misses other server text in the same items
The comment says server text is made printable, but only name, review_reason, reviewed_by_name and proposed_by_name are converted; the spread still carries description (a nullable server string on Contribution) and ids raw into JSON that a client or agent reads. printableDeep exists for exactly this ("for JSON a client or agent reads") — replacing the hand-maintained field list with printableDeep(item) covers every field and cannot drift.
| @@ -67,7 +67,7 @@ With workspaces on: | |||
| - `/integrations` in the TUI opens the workspace's integration status directly (the same view as `/workspace` → Status). In a project that is not linked it offers the link, and when the workspace engine is missing or too old it offers the install and says which. | |||
| - In a project linked to a workspace, integrations come only from the workspace's engine, so the `datamate_manager` tool is not offered there and refuses to run. `datamate-*` MCP servers already in the config still load, and a one-time notice names the file they are in; the config is not edited. | |||
There was a problem hiding this comment.
SUGGESTION: the new workspace review decisions command is undocumented
This paragraph documents the review CLI and now mentions /rules and the serve routes, but not altimate-code workspace review decisions --json [--unseen] [--mark-seen] [--host H] that this PR adds for the Claude Code plugin, nor its deliberate always-JSON/always-exit-0 contract (which differs from the general "ok is true exactly when the exit code is 0" statement above). Add it so the CLI reference matches the implementation.
Code Review SummaryStatus: 15 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Reviewed by deepseek-v4.1-flash-1017 · Input: 130K · Output: 30.3K · Cached: 1.5M Review guidance: REVIEW.md from base branch |
dev-punia-altimate
left a comment
dev-punia-altimate
left a comment
There was a problem hiding this comment.
🤖 Code Review — OpenCodeReview (Gemini) — 11 finding(s)
- 11 anchored to a line (posted inline when the comment stream is on)
- 0 without a line anchor
All findings (full text)
1. packages/opencode/src/altimate/workspace/skill-publish.ts (L1086-L1097)
[🟠 MEDIUM] The codes array is static and currently being recreated along with its 9 inner tuples on every invocation of publishErrorCode. Consider moving it outside of the function to avoid unnecessary memory allocations and improve performance, especially if this function might be called frequently.
Suggested change:
const ERROR_CODES: [new (...args: never[]) => Error, string][] = [
[ReviewDisabledError, "review_disabled"],
[TooManyReviewRequestsError, "too_many_requests"],
[SubmitFailedError, "submit_failed"],
[NotLinkedError, "not_linked"],
[CredentialsChangedError, "credentials_changed"],
[NotWorkspaceOwnerError, "not_workspace_owner"],
[SkillNameConflictError, "name_conflict"],
[SkillChangedElsewhereError, "changed_elsewhere"],
[AttachFailedError, "attach_failed"],
];
export function publishErrorCode(err: unknown): string | null {
return ERROR_CODES.find(([type]) => err instanceof type)?.[1] ?? null;
2. packages/opencode/src/altimate/rules/store.ts (L42)
[🔵 LOW] This insertion separates the JSDoc comment from the hostNoticeKey function it is meant to describe. As a result, the comment is now incorrectly associated with this new constant. Consider moving MAX_HOST_REVIEW_SEEN above the JSDoc comment or giving it its own dedicated comment.
3. packages/opencode/src/altimate/rules/store.ts (L444-L445)
[🔵 LOW] There is a mismatch between the comment and the code. The comment mentions keeping "50 decisions", but MAX_HOST_REVIEW_SEEN is defined as 200. Please update the comment to match the actual behavior (200), or adjust the constant value if 50 was intended.
Suggested change:
// Newest kept: the review state keeps 200 decisions, reported at most once each within NOTICE_MAX_AGE_MS.
next.host_review_seen = next.host_review_seen.slice(-MAX_HOST_REVIEW_SEEN)
4. packages/opencode/src/altimate/workspace/review-requests.ts (L127)
[🟠 MEDIUM] The self-referential export export * as ReviewRequests from "./review-requests" creates a circular dependency where the module exports itself as a namespace. This is generally considered an anti-pattern and can cause unpredictable behavior or infinite loops in certain bundlers or transpilers.
If consumers need a namespace, they should use namespace imports (import * as ReviewRequests from "./review-requests" or const ReviewRequests = await import("./review-requests")) rather than having the module export itself.
5. packages/opencode/src/cli/cmd/workspace.ts (L507-L531)
[🔴 HIGH] The comment specifies always JSON, always exit 0. However, the handler lacks a try/catch block. If ReviewNotices.hostDecisions() or the dynamic import throws an exception (e.g., due to network issues or missing configuration), it will result in an unhandled promise rejection. This typically causes the CLI framework to output a raw stack trace to stderr and exit with a non-zero status code, breaking the JSON output guarantee for editor and agent plugins.
To fix this, wrap the logic in a try/catch block and output the error as a JSON payload, ensuring process.exitCode = 0 is still set. You can utilize the local messageOf function for consistent error formatting.
Suggested change:
handler: async (args) => {
try {
const { ReviewNotices } = await import("@/altimate/workspace/review-notices")
const result = await ReviewNotices.hostDecisions(args.directory ?? process.cwd(), {
...(args.host ? { host: args.host } : {}),
unseen: args.unseen,
markSeen: args["mark-seen"],
})
const body = result.ok
? {
ok: true,
decisions: result.decisions.map((d) => ({
kind: d.kind,
public_id: d.public_id,
name: printable(d.name),
status: d.status,
at: d.at,
...(d.reason ? { reason: printable(d.reason) } : {}),
...(d.reviewer ? { reviewer: printable(d.reviewer) } : {}),
})),
waiting: result.waiting,
}
: result
process.stdout.write(JSON.stringify(body) + "\n")
} catch (err) {
process.stdout.write(JSON.stringify({ ok: false, error: messageOf(err) }) + "\n")
}
process.exitCode = 0
},
6. packages/opencode/src/altimate/rules/service.ts (L109-L113)
[🔵 LOW] The insertion of pollReviewDecisions and lastKept here separates the existing JSDoc comment (/** Sync now, or join the sync already running... */) from its target function syncNow. As a result, syncNow loses its documentation, and lastKept incorrectly inherits multiple stacked block comments. Please move the JSDoc for syncNow down so it immediately precedes the syncNow function.
7. packages/opencode/src/altimate/rules/service.ts (L115-L116)
[🔵 LOW] The dynamic import await import("../workspace/review-notices") is executed on every invocation of pollReviewDecisions. While modern JS engines cache module resolutions, top-level static imports are generally preferred for better readability and slightly better execution performance, unless this dynamic import is intentionally used to lazy-load or prevent a circular dependency.
Suggested change:
export async function pollReviewDecisions(directory: string, opts: { announceChange?: boolean } = {}) {
const { ReviewNotices } = await import("../workspace/review-notices")
8. packages/opencode/src/altimate/rules/service.ts (L132-L134)
[🔵 LOW] Although notices is currently typed with a default array value in the Zod schema, using optional chaining (?.notices?.at(-1)) is a safer defensive programming practice. It prevents a potential TypeError if the parsed state structure changes in the future or if notices ever evaluates to undefined.
Suggested change:
if (result.status === "done") {
const newest = (await ReviewNotices.storedState(directory))?.notices?.at(-1)
const key = newest ? `${newest.kind}:${newest.public_id}:${newest.status}` : ""
9. packages/opencode/src/plugin/tui/altimate/rules.tsx (L528-L531)
[🟠 MEDIUM] The setTimeout is created anonymously and is never cleared if the get request resolves first. This leaves a timer running in the Node.js event loop for up to 5 seconds on every menu interaction, consuming unnecessary resources and potentially preventing the process from exiting gracefully. Consider storing the timer reference and clearing it when the API call finishes.
Suggested change:
let timer: NodeJS.Timeout
const listed = await Promise.race([
get<RequestsResponse>(api, "/altimate/workspace/requests").finally(() => clearTimeout(timer)),
new Promise<"slow">((resolve) => { timer = setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS) }),
])
10. packages/opencode/src/plugin/tui/altimate/rules.tsx (L670-L672)
[🟠 MEDIUM] When the user selects the 'Cancel' option, api.ui.dialog.clear() is called and the function returns early. This dismisses the entire dialog stack rather than returning the user to the main rules list. Consider calling showMenu(api) to re-render the previous view instead of closing the dialog completely.
Suggested change:
onSelect={async (option) => {
if (option.value === "cancel") return showMenu(api)
api.ui.dialog.clear()
if (option.value !== "withdraw" && option.value !== "remove") return
11. packages/opencode/src/altimate/workspace/review-notices.ts (L395-L397)
[🟠 MEDIUM] Because decisions is reversed to be newest-first, the added array inherits this newest-first order.\n\nWhen added is appended to kept and truncated via .slice(-MAX_HOST_SEEN), it keeps the end of the combined array. Since added is newest-first, the end of the array actually contains the oldest notices of the newly added batch. Furthermore, within each batch appended to kept, the newest items are placed at the beginning, meaning they are the first to be dropped when kept exceeds MAX_HOST_SEEN.\n\nAlthough currently masked because state.notices is capped at MAX_NOTICES (50) which is less than MAX_HOST_SEEN (200), this is a latent logic flaw. If added were ever larger than MAX_HOST_SEEN, this would keep the oldest items and drop the newest ones.\n\nSuggestion: Reverse the added array before appending it to kept so it is oldest-first. This ensures the newest notices are placed at the very end and are preserved during .slice(-MAX_HOST_SEEN).
Suggested change:
const kept = current.host_seen[host] ?? []
const added = decisions.map(key).filter((k) => !kept.includes(k)).reverse()
return { state: { ...current, host_seen: { ...current.host_seen, [host]: [...kept, ...added].slice(-MAX_HOST_SEEN) } }, result: undefined }
| const codes: [new (...args: never[]) => Error, string][] = [ | ||
| [ReviewDisabledError, "review_disabled"], | ||
| [TooManyReviewRequestsError, "too_many_requests"], | ||
| [SubmitFailedError, "submit_failed"], | ||
| [NotLinkedError, "not_linked"], | ||
| [CredentialsChangedError, "credentials_changed"], | ||
| [NotWorkspaceOwnerError, "not_workspace_owner"], | ||
| [SkillNameConflictError, "name_conflict"], | ||
| [SkillChangedElsewhereError, "changed_elsewhere"], | ||
| [AttachFailedError, "attach_failed"], | ||
| ] | ||
| return codes.find(([type]) => err instanceof type)?.[1] ?? null |
There was a problem hiding this comment.
[🟠 MEDIUM] The codes array is static and currently being recreated along with its 9 inner tuples on every invocation of publishErrorCode. Consider moving it outside of the function to avoid unnecessary memory allocations and improve performance, especially if this function might be called frequently.
Suggested change:
| const codes: [new (...args: never[]) => Error, string][] = [ | |
| [ReviewDisabledError, "review_disabled"], | |
| [TooManyReviewRequestsError, "too_many_requests"], | |
| [SubmitFailedError, "submit_failed"], | |
| [NotLinkedError, "not_linked"], | |
| [CredentialsChangedError, "credentials_changed"], | |
| [NotWorkspaceOwnerError, "not_workspace_owner"], | |
| [SkillNameConflictError, "name_conflict"], | |
| [SkillChangedElsewhereError, "changed_elsewhere"], | |
| [AttachFailedError, "attach_failed"], | |
| ] | |
| return codes.find(([type]) => err instanceof type)?.[1] ?? null | |
| const ERROR_CODES: [new (...args: never[]) => Error, string][] = [ | |
| [ReviewDisabledError, "review_disabled"], | |
| [TooManyReviewRequestsError, "too_many_requests"], | |
| [SubmitFailedError, "submit_failed"], | |
| [NotLinkedError, "not_linked"], | |
| [CredentialsChangedError, "credentials_changed"], | |
| [NotWorkspaceOwnerError, "not_workspace_owner"], | |
| [SkillNameConflictError, "name_conflict"], | |
| [SkillChangedElsewhereError, "changed_elsewhere"], | |
| [AttachFailedError, "attach_failed"], | |
| ]; | |
| export function publishErrorCode(err: unknown): string | null { | |
| return ERROR_CODES.find(([type]) => err instanceof type)?.[1] ?? null; |
| const MAX_HOST_SESSIONS = 50 | ||
|
|
||
| /** The key a host session start marks a review notice reported under (`host_notices_seen`). */ | ||
| export const MAX_HOST_REVIEW_SEEN = 200 |
There was a problem hiding this comment.
[🔵 LOW] This insertion separates the JSDoc comment from the hostNoticeKey function it is meant to describe. As a result, the comment is now incorrectly associated with this new constant. Consider moving MAX_HOST_REVIEW_SEEN above the JSDoc comment or giving it its own dedicated comment.
| // Newest kept: the review state keeps 50 decisions, reported at most once each within NOTICE_MAX_AGE_MS. | ||
| next.host_review_seen = next.host_review_seen.slice(-MAX_HOST_REVIEW_SEEN) |
There was a problem hiding this comment.
[🔵 LOW] There is a mismatch between the comment and the code. The comment mentions keeping "50 decisions", but MAX_HOST_REVIEW_SEEN is defined as 200. Please update the comment to match the actual behavior (200), or adjust the constant value if 50 was intended.
Suggested change:
| // Newest kept: the review state keeps 50 decisions, reported at most once each within NOTICE_MAX_AGE_MS. | |
| next.host_review_seen = next.host_review_seen.slice(-MAX_HOST_REVIEW_SEEN) | |
| // Newest kept: the review state keeps 200 decisions, reported at most once each within NOTICE_MAX_AGE_MS. | |
| next.host_review_seen = next.host_review_seen.slice(-MAX_HOST_REVIEW_SEEN) |
| return { ready: true, actAs, datamateId: outcome.binding.datamateId, name: printable(outcome.binding.datamateName) } | ||
| } | ||
|
|
||
| export * as ReviewRequests from "./review-requests" |
There was a problem hiding this comment.
[🟠 MEDIUM] The self-referential export export * as ReviewRequests from "./review-requests" creates a circular dependency where the module exports itself as a namespace. This is generally considered an anti-pattern and can cause unpredictable behavior or infinite loops in certain bundlers or transpilers.
If consumers need a namespace, they should use namespace imports (import * as ReviewRequests from "./review-requests" or const ReviewRequests = await import("./review-requests")) rather than having the module export itself.
| handler: async (args) => { | ||
| const { ReviewNotices } = await import("@/altimate/workspace/review-notices") | ||
| const result = await ReviewNotices.hostDecisions(args.directory ?? process.cwd(), { | ||
| ...(args.host ? { host: args.host } : {}), | ||
| unseen: args.unseen, | ||
| markSeen: args["mark-seen"], | ||
| }) | ||
| const body = result.ok | ||
| ? { | ||
| ok: true, | ||
| decisions: result.decisions.map((d) => ({ | ||
| kind: d.kind, | ||
| public_id: d.public_id, | ||
| name: printable(d.name), | ||
| status: d.status, | ||
| at: d.at, | ||
| ...(d.reason ? { reason: printable(d.reason) } : {}), | ||
| ...(d.reviewer ? { reviewer: printable(d.reviewer) } : {}), | ||
| })), | ||
| waiting: result.waiting, | ||
| } | ||
| : result | ||
| process.stdout.write(JSON.stringify(body) + "\n") | ||
| process.exitCode = 0 | ||
| }, |
There was a problem hiding this comment.
[🔴 HIGH] The comment specifies always JSON, always exit 0. However, the handler lacks a try/catch block. If ReviewNotices.hostDecisions() or the dynamic import throws an exception (e.g., due to network issues or missing configuration), it will result in an unhandled promise rejection. This typically causes the CLI framework to output a raw stack trace to stderr and exit with a non-zero status code, breaking the JSON output guarantee for editor and agent plugins.
To fix this, wrap the logic in a try/catch block and output the error as a JSON payload, ensuring process.exitCode = 0 is still set. You can utilize the local messageOf function for consistent error formatting.
Suggested change:
| handler: async (args) => { | |
| const { ReviewNotices } = await import("@/altimate/workspace/review-notices") | |
| const result = await ReviewNotices.hostDecisions(args.directory ?? process.cwd(), { | |
| ...(args.host ? { host: args.host } : {}), | |
| unseen: args.unseen, | |
| markSeen: args["mark-seen"], | |
| }) | |
| const body = result.ok | |
| ? { | |
| ok: true, | |
| decisions: result.decisions.map((d) => ({ | |
| kind: d.kind, | |
| public_id: d.public_id, | |
| name: printable(d.name), | |
| status: d.status, | |
| at: d.at, | |
| ...(d.reason ? { reason: printable(d.reason) } : {}), | |
| ...(d.reviewer ? { reviewer: printable(d.reviewer) } : {}), | |
| })), | |
| waiting: result.waiting, | |
| } | |
| : result | |
| process.stdout.write(JSON.stringify(body) + "\n") | |
| process.exitCode = 0 | |
| }, | |
| handler: async (args) => { | |
| try { | |
| const { ReviewNotices } = await import("@/altimate/workspace/review-notices") | |
| const result = await ReviewNotices.hostDecisions(args.directory ?? process.cwd(), { | |
| ...(args.host ? { host: args.host } : {}), | |
| unseen: args.unseen, | |
| markSeen: args["mark-seen"], | |
| }) | |
| const body = result.ok | |
| ? { | |
| ok: true, | |
| decisions: result.decisions.map((d) => ({ | |
| kind: d.kind, | |
| public_id: d.public_id, | |
| name: printable(d.name), | |
| status: d.status, | |
| at: d.at, | |
| ...(d.reason ? { reason: printable(d.reason) } : {}), | |
| ...(d.reviewer ? { reviewer: printable(d.reviewer) } : {}), | |
| })), | |
| waiting: result.waiting, | |
| } | |
| : result | |
| process.stdout.write(JSON.stringify(body) + "\n") | |
| } catch (err) { | |
| process.stdout.write(JSON.stringify({ ok: false, error: messageOf(err) }) + "\n") | |
| } | |
| process.exitCode = 0 | |
| }, |
| export async function pollReviewDecisions(directory: string, opts: { announceChange?: boolean } = {}) { | ||
| const { ReviewNotices } = await import("../workspace/review-notices") |
There was a problem hiding this comment.
[🔵 LOW] The dynamic import await import("../workspace/review-notices") is executed on every invocation of pollReviewDecisions. While modern JS engines cache module resolutions, top-level static imports are generally preferred for better readability and slightly better execution performance, unless this dynamic import is intentionally used to lazy-load or prevent a circular dependency.
Suggested change:
| export async function pollReviewDecisions(directory: string, opts: { announceChange?: boolean } = {}) { | |
| const { ReviewNotices } = await import("../workspace/review-notices") | |
| export async function pollReviewDecisions(directory: string, opts: { announceChange?: boolean } = {}) { | |
| const { ReviewNotices } = await import("../workspace/review-notices") |
| if (result.status === "done") { | ||
| const newest = (await ReviewNotices.storedState(directory))?.notices.at(-1) | ||
| const key = newest ? `${newest.kind}:${newest.public_id}:${newest.status}` : "" |
There was a problem hiding this comment.
[🔵 LOW] Although notices is currently typed with a default array value in the Zod schema, using optional chaining (?.notices?.at(-1)) is a safer defensive programming practice. It prevents a potential TypeError if the parsed state structure changes in the future or if notices ever evaluates to undefined.
Suggested change:
| if (result.status === "done") { | |
| const newest = (await ReviewNotices.storedState(directory))?.notices.at(-1) | |
| const key = newest ? `${newest.kind}:${newest.public_id}:${newest.status}` : "" | |
| if (result.status === "done") { | |
| const newest = (await ReviewNotices.storedState(directory))?.notices?.at(-1) | |
| const key = newest ? `${newest.kind}:${newest.public_id}:${newest.status}` : "" |
| const listed = await Promise.race([ | ||
| get<RequestsResponse>(api, "/altimate/workspace/requests"), | ||
| new Promise<"slow">((resolve) => setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS)), | ||
| ]) |
There was a problem hiding this comment.
[🟠 MEDIUM] The setTimeout is created anonymously and is never cleared if the get request resolves first. This leaves a timer running in the Node.js event loop for up to 5 seconds on every menu interaction, consuming unnecessary resources and potentially preventing the process from exiting gracefully. Consider storing the timer reference and clearing it when the API call finishes.
Suggested change:
| const listed = await Promise.race([ | |
| get<RequestsResponse>(api, "/altimate/workspace/requests"), | |
| new Promise<"slow">((resolve) => setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS)), | |
| ]) | |
| let timer: NodeJS.Timeout | |
| const listed = await Promise.race([ | |
| get<RequestsResponse>(api, "/altimate/workspace/requests").finally(() => clearTimeout(timer)), | |
| new Promise<"slow">((resolve) => { timer = setTimeout(() => resolve("slow"), REQUESTS_WAIT_MS) }), | |
| ]) |
| onSelect={async (option) => { | ||
| api.ui.dialog.clear() | ||
| if (option.value !== "withdraw" && option.value !== "remove") return |
There was a problem hiding this comment.
[🟠 MEDIUM] When the user selects the 'Cancel' option, api.ui.dialog.clear() is called and the function returns early. This dismisses the entire dialog stack rather than returning the user to the main rules list. Consider calling showMenu(api) to re-render the previous view instead of closing the dialog completely.
Suggested change:
| onSelect={async (option) => { | |
| api.ui.dialog.clear() | |
| if (option.value !== "withdraw" && option.value !== "remove") return | |
| onSelect={async (option) => { | |
| if (option.value === "cancel") return showMenu(api) | |
| api.ui.dialog.clear() | |
| if (option.value !== "withdraw" && option.value !== "remove") return |
| const kept = current.host_seen[host] ?? [] | ||
| const added = decisions.map(key).filter((k) => !kept.includes(k)) | ||
| return { state: { ...current, host_seen: { ...current.host_seen, [host]: [...kept, ...added].slice(-MAX_HOST_SEEN) } }, result: undefined } |
There was a problem hiding this comment.
[🟠 MEDIUM] Because decisions is reversed to be newest-first, the added array inherits this newest-first order.\n\nWhen added is appended to kept and truncated via .slice(-MAX_HOST_SEEN), it keeps the end of the combined array. Since added is newest-first, the end of the array actually contains the oldest notices of the newly added batch. Furthermore, within each batch appended to kept, the newest items are placed at the beginning, meaning they are the first to be dropped when kept exceeds MAX_HOST_SEEN.\n\nAlthough currently masked because state.notices is capped at MAX_NOTICES (50) which is less than MAX_HOST_SEEN (200), this is a latent logic flaw. If added were ever larger than MAX_HOST_SEEN, this would keep the oldest items and drop the newest ones.\n\nSuggestion: Reverse the added array before appending it to kept so it is oldest-first. This ensures the newest notices are placed at the very end and are preserved during .slice(-MAX_HOST_SEEN).
Suggested change:
| const kept = current.host_seen[host] ?? [] | |
| const added = decisions.map(key).filter((k) => !kept.includes(k)) | |
| return { state: { ...current, host_seen: { ...current.host_seen, [host]: [...kept, ...added].slice(-MAX_HOST_SEEN) } }, result: undefined } | |
| const kept = current.host_seen[host] ?? [] | |
| const added = decisions.map(key).filter((k) => !kept.includes(k)).reverse() | |
| return { state: { ...current, host_seen: { ...current.host_seen, [host]: [...kept, ...added].slice(-MAX_HOST_SEEN) } }, result: undefined } |
🤖 Code Review — OpenCodeReview (Gemini) — 11 finding(s)
All findings (full text)1.
|
Stacked on #1708 (withdraw and remove). This brings workspace review to clients other than the TUI: the VS Code extension and the Claude Code and Codex plugins. It also adds withdraw and remove to the TUI.
What changes
GET /altimate/workspace/requests: your own requests on the linked workspace in one list.withdrawa waiting one,removean approved one).review_statussays why skills and knowledge are missing when they are.POST /altimate/workspace/requests/{id}/withdraw|remove: acts on one of your skill or knowledge requests. Refusals keep the server's code, and clients are told to refresh.decisionsin the rules summary: rule reviews and skill and knowledge decisions in one list, newest first, keyedkind:public_id:status.noticesandreview_noticesstay for released clients./rules:learn hook session-start):host_review_seen) and how many requests are still waiting.POST /altimate/rules/sync) also checks skill and knowledge decisions (pollReviewDecisions), so an approval or removal shows at once.workspace review decisions --json [--unseen] [--mark-seen] [--host H]: for the Claude Code plugin's toast. It always answers in JSON and exits 0.skill publishrefusals carry a stablecode:review_disabled,too_many_requests, and others.workspace review status --jsonis made printable.Verification
decisionsper host (including a linked person with nothing decided yet), "Sync now" polling, and the route checks./rules, withdraw, an owner removal noticed through "Sync now", and an author removal.decisions, decisions recorded by another process not reaching a running client, a/rulesfetch with no time limit, and unsanitised text. All are fixed in 0d97fd5./rules(extension)Clients: AltimateAI/vscode-altimate-mcp-server#545, AltimateAI/altimate-claude-plugin#22, AltimateAI/altimate-codex-plugin#8.
🤖 Generated with Claude Code