Skip to content

fix(mt#5031): Split what just happened from what is currently true in the Detail column - #3676

Merged
edobry merged 4 commits into
mainfrom
task/mt-5031
Sep 8, 2026
Merged

edobry merged 4 commits into
mainfrom
task/mt-5031

Conversation

@minsky-ai

@minsky-ai minsky-ai Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

The cockpit's providers table rendered lastValidationDetail in its Detail column. That string is
written for transient post-action feedback — a sentence shown inline, untruncated, to someone
who just pressed Validate or Add. The column asks a different question ("what is currently true of
this credential") and is 276px wide, so two providers' strings were silently cut to roughly half.

The principal reported it after storing a subscription token:

"in the detail column for the claude code one, I see this text, which is both cut off and also weird"

This is mt#5027's AT4 acceptance judgment, and it is negative. That task deferred the copy's
acceptance to the principal's reading; this is that reading. Its wording is correct for the surface
it was designed for and wrong for the surface the principal saw.

It was a class, and the reported instance was not the worst one

Measured live against the operator's cockpit before the change:

provider chars needs column truncated
telegram 95 523px 276px yes, ~47% lost
claude-code-token 84 465px 276px yes, ~41% lost
github 46 276px 276px no — exactly at the boundary
railway / google / supabase 18–20 276px 276px no

Telegram's is longer than the reported one and is an instruction ("send your bot one message
so…"), which is even less appropriate for a status column. A claude-code-token-only fix would
have repeated mt#5027's original mistake of treating a class as an instance.

Why shortening the copy could not have worked

mt#5027 bounded the new string at "under 15 words". The shipped string is ~13 words — it passed
— and 84 characters. The column is bounded in pixels. The unit was wrong, so the check that
existed to prevent this could not see it.

Key Changes

  • CredentialCheckResult.status — a short state form beside the existing detail, persisted as
    lastValidationStatus. Optional, and absence is safe by construction.
  • credentialStatusLine (web/lib/credentials-api.ts) — the column now reads this and never
    lastValidationDetail. Five rungs: provider status → nothing for a presence-only row → nothing
    for an unconfigured one → a short stored detail (back-compat) → a derived state. A provider
    that supplies no status, or a stored detail over budget, cannot truncate the column
    — that is
    the structural guarantee, not a convention.
  • All eight providers supply a status. Two got distinct ones (stored, unverified;
    valid; no chats discovered yet); the rest already read as states and repeat theirs.
  • MAX_STATUS_LENGTH = 40, derived from the 276px measurement rather than chosen.
  • listPresence() keeps carrying detail, with the reasoning recorded in code: nothing
    truncates that payload, so the constraint motivating status does not apply there.
  • The full sentence stays reachable as a title tooltip, and is unchanged on both transient
    surfaces (Settings form and the masked entry form on a credential-request ask).

Spec reconciliation: the budget is 40, where SC1 says ≤ 46

SC1 derives a ≤46-char working budget from the measurement. I implemented 40, which satisfies
that criterion (strictly tighter) but is a deliberate departure worth naming, because it has a
visible consequence. 46 is the ceiling at a 1440px viewport with zero headroom — the measured
GitHub row needed exactly the full 276px — so a 46-char value truncates in any narrower window. 40
leaves room.

The cost: GitHub's stored pre-split detail is 46 chars, so the back-compat rung declines it and
that row reads validated until its next recheck. Recorded in ## Live verification below rather
than left to be discovered.

Testing

Execution evidence:

$ bun test --preload ./tests/setup.ts packages/domain/src/credentials/providers/status-line.test.ts
 11 pass  0 fail  28 expect() calls   Ran 11 tests across 1 file.

$ bun test <components invocation> ./src/cockpit/web/lib/credentials-api.test.ts \
                                   ./src/cockpit/web/widgets/Credentials.test.tsx
 25 pass  0 fail  Ran 25 tests across 2 files.

$ bun test --preload ./tests/setup.ts packages/domain/src/credentials/
 222 pass  0 fail  468 expect() calls   Ran 222 tests across 13 files.

$ validate_typecheck  -> pass, 8 projects, 0 errors
$ validate_lint       -> pass, 0 errors, 0 warnings, 4412 files

AT1 — rendered the real table at 1440x1000 and measured scrollWidth vs clientWidth per cell
rather than eyeballing a screenshot: zero rows truncated (see ## Live verification). This is
SC1's mechanical half.

AT2 — the transient surface still shows the sentence in full (SC3): drives the form and
asserts the 84-char string is rendered complete inline. This is the half a "just shorten the copy"
fix would break while passing every column assertion.

AT3 — Credentials widget — Detail column states: all four states through the rendered row —
provider status, pre-split long detail, configured-never-checked (SC4), presence-only. Plus a
document-wide negative that the 84-char prose appears in no cell at all, and a title assertion
for SC6.

AT4 — read back GET /api/credentials from a cockpit running this build; every column-bound
value is a state. Numbers in ## Live verification.

AT5 — principal's reading. Outstanding by design; this is subjective-quality acceptance and
theirs, so the render is presented without a verdict.

SC2 is structural, per credentialStatusLine above, plus a class sweep asserting every
provider's status literal is within budget. SC5 is the back-compat rung, tested with the two
real pre-split strings and documented in CredentialMeta. SC7 is recorded in
request-resolver.ts.

Negative control — the render rule: reverted credentialStatusLine to the full pre-fix behaviour
(return the stored detail directly). 5 of 10 failed, including "the two REAL over-budget details
are never rendered" and "every value fits the budget". Rungs 2–4 passed under the control, because
the old code coincidentally agreed on those inputs — so those three assertions are not
discriminating on their own, and I am not claiming them as such.

Negative control — the component wiring: reverted CredentialRow to render lastValidationDetail
directly. Both new component tests failed. This is the control that matters for the actual
defect: a green rule test is compatible with a row that never calls the rule.

Live verification

Built and ran this branch's cockpit from the session workspace on :3941 — health body confirms
"service":"minsky-cockpit" and "commit":"6f664fd15", so the probe is against this build, not the
operator's. Same CDP measurement as the before-table, same viewport, the operator's real credential
store.

provider before after truncated after
claude-code-token 84 chars, cut at 276/465px validated no
github authenticated as @edobry; …scope present validated no
anthropic (empty cell) configured, never checked no
supabase 4 projects visible 4 projects visible no
google 50 models accessible 50 models accessible no
railway railway:Eugene Dobry railway:Eugene Dobry no

Zero truncated rows. Two things in that table are worth stating plainly rather than leaving to
be noticed:

  • github reads validated rather than its identity line — the budget consequence described
    under Spec reconciliation. It self-heals to authenticated as @edobry on that credential's next
    recheck. I did not force a recheck: that writes to the operator's credential store and makes live
    API calls with their tokens, which this task does not authorize.
  • telegram could not be verified live, and its row is absent above for that reason. In the
    session environment its isConfigured() shells out to Pulumi with no stack selected, so it reads
    configured: false (confirmed: false on :3941 vs true on the operator's :3737) and takes the
    unconfigured rung. My live probe therefore cannot show telegram's real post-fix rendering —
    that path is covered by unit tests using the real 95-char string, not by this run.

Deploy verification: every changed file returns true from isDeploySurfaceFile (ran the
predicate over the diff rather than recalling the pattern set), so this is deploy surface and §10
applies. I will confirm the post-merge deploy with notBefore = the merge timestamp and
expectCommitSha = the merge sha, and re-read the rendered column on the operator's cockpit once
the tray rebuild picks it up.

Notes

The reported bug's sibling — a UI flicker when pressing Add, reported in the same message — is
filed separately as mt#5032 and deliberately not touched here. Both edit
src/cockpit/web/widgets/Credentials.tsx, so they should land sequentially.

PR #3412 (the 614-site logging sweep) also touches request-resolver.ts, in a different function
~25 lines from this change. No line-level overlap; whichever lands second rebases trivially.

The providers table's Detail column rendered `lastValidationDetail` —
a string written for transient post-action feedback. Two providers put
sentences there (claude-code-token 84 chars, telegram 95) against a
276px column, so both were truncated to roughly half.

Adds `CredentialCheckResult.status`, a short state form persisted
alongside the detail, and routes the column through
`credentialStatusLine`, which can never truncate: a provider that
supplies no status, or a stored detail over budget, falls through to a
derived state. The full sentence stays reachable as a tooltip and is
unchanged on the transient surfaces.
… level

The rule test proves credentialStatusLine returns the right text; it
cannot prove CredentialRow asks it. That wiring is the defect that
shipped, so assert it through the rendered row — including that the
84-char prose appears nowhere in the table, and that the full sentence
stays reachable as a tooltip.
…in full

The split has two halves and only one of them was guarded. A fix that
"shortened the copy" would pass every column assertion while breaking
the surface mt#5027's wording was accepted for, so assert that the same
string the column refuses is rendered complete inline.
@minsky-ai minsky-ai Bot added the authorship/co-authored Co-authored by human and AI agent label Sep 8, 2026
@minsky-reviewer

minsky-reviewer Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Minsky Reviewer Status

Verdict: APPROVED — no blocking findings
Review: View review
Model: openai/gpt-5 | Tokens: 314K prompt, 7K completion | Duration: 83s
Mode: normal

Commands

  • /review — request a fresh review

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Thorough cross-cut addressing the conflation between transient feedback and status rendering. Domain types gained status, providers populate it, persistence carries lastValidationStatus, and the cockpit renders via a new credentialStatusLine with comprehensive tests. The back-compat rungs and tooltip retention minimize regressions, and unit coverage includes both the rule and the component wiring.

Findings are non-blocking: (1) MAX_STATUS_LENGTH is duplicated across domain and web, creating drift risk; add a guard test or shared constant. (2) /api/credentials/validate omits the new status even when providers supply it; clarify intent or include it for symmetry. (3) The status-literal sweep intentionally skips property-access/dynamic cases; consider a small runtime seam test for those. (4) Tooltip uses lastValidationDetail directly; safe but consider explicit ?? undefined and documenting reliance. With these nits addressed or documented, the PR is solid to merge.

Findings

  • [NON-BLOCKING] src/cockpit/web/lib/credentials-api.ts:66 — Constant MAX_STATUS_LENGTH is duplicated client- and server-side, creating drift risk
    src/cockpit/web/lib/credentials-api.ts:66-74 redefines MAX_STATUS_LENGTH = 40 to mirror packages/domain/src/credentials/types.ts:14-34. The comment calls out the duplication as convention, but this still creates a silent-divergence risk if one side is changed and the other is missed. Consider: (a) exporting a tiny shared constants module owned by domain but imported into web (still type-only), (b) generating the client copy from the domain source as part of build, or (c) deriving from a single CSS-driven measurement surface exposed as a test util. At minimum, add a test that asserts the two constants remain equal to catch drift.
  • [NON-BLOCKING] src/cockpit/routes/credentials.ts:84 — /api/credentials/validate response omits status even when providers now supply it
    Providers now optionally return a status on successful checks (e.g., packages/domain/src/credentials/providers/*.ts), and addCredential persists it to lastValidationStatus. The POST /api/credentials/validate route at src/cockpit/routes/credentials.ts:73-107 still only returns { ok, detail, unauthorized?, scopeGap? }. If callers might want to preview the concise state string before persisting, consider returning status alongside detail for symmetry with the domain CredentialCheckResult. If omission is intentional (transient surface should always show full detail), document that here.
  • [NON-BLOCKING] packages/domain/src/credentials/providers/status-line.test.ts:70 — Static-literal extractor skips property-access initializers; very-long dynamic interpolations can exceed budget undetected
    The test intentionally excludes cases like status: userCheck.status and treats dynamic spans as residuals (see providers/status-line.test.ts docblock). This leaves two gaps: (1) an upstream status flowing through property access can exceed the budget and pass this test; (2) very long dynamic parts (e.g., unusually long usernames in github or railway) can exceed 40 chars. The widget's truncate backstop mitigates (2), but (1) is still unguarded by this suite. Consider adding a runtime seam test for known property-access paths (e.g., github's scope-present branch) to assert typical lengths or to ensure the column rule still clamps via credentialStatusLine.
  • [NON-BLOCKING] src/cockpit/web/widgets/Credentials.tsx:325 — title tooltip uses lastValidationDetail even when null/undefined; consider defensive default
    <span ... title={listing.lastValidationDetail}> may receive undefined when a row lacks a stored detail (pre-split but never-validated, or schema-only rows if wiring changes). React will omit the attribute, which is safe — just flagging that the tooltip guarantee in SC6 relies on lastValidationDetail being present on rows that render a state. If a provider supplies only a status and no detail (or if detail is later refactored away from persistence), the tooltip would be empty. Consider explicitly using title={listing.lastValidationDetail ?? undefined} (to be explicit) and documenting the reliance on persisted detail.

Documentation impact

  • no-update-needed — The change introduces an internal split between detail and status for credential check results and adjusts the cockpit rendering logic. The PR description already notes docs/** has no references to /api/credentials or these fields, and a spot-check of src/cockpit/routes/credentials.ts shows the API surface is unchanged for GET /api/credentials (it now includes lastValidationStatus in the listing but this API is not documented). No user-facing CLI/docs were modified or invalidated. If there is an internal API doc for the cockpit JSON, it was not found in-repo; otherwise no docs updates are needed.

@github-actions

github-actions Bot commented Sep 8, 2026 •

Copy link
Copy Markdown

Cockpit Preview

Preview URL: https://cockpit-preview-production.up.railway.app

Deployed from commit b5b7b1146.
Preview runs in read-only mode — mutations are disabled.

Updated by Cockpit Preview.

…ed budget

Two reviewer findings from PR #3676, both real.

The validate endpoint narrows its response to an explicit allowlist and
never listed `status`, so the client type declared a field that route
could not populate. Added to the allowlist rather than switching to a
spread — the narrowing is deliberate.

The web-side MAX_STATUS_LENGTH mirrors the domain constant and could
drift. Importing the domain into the cockpit-web test was tried and
reverted: it pulls the barrel into that TypeScript project, where
telegram.ts does not typecheck — which is the reason the mirror
convention exists. The guard reads the literal instead.

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Re-verification complete on the fix commit. The prior non-blocking issues were addressed: src/cockpit/routes/credentials.ts now includes status in the validate-route allowlist, and the cockpit/web constant drift risk is actively guarded by a test that parses the mirrored literal. The structural split between transient detail and persistent state is correctly implemented: the Detail column now renders credentialStatusLine, providers supply bounded status values, and presence/test surfaces continue to show full detail. Back-compat rungs and the ask-side listPresence() decision are explicit in code. I found no new blocking defects; one minor note about the brittleness of the hard-coded test path is non-blocking. Verdict: APPROVE.

Findings

  • [NON-BLOCKING] packages/domain/src/credentials/providers/status-line.test.ts:223 — Test reads mirrored constant from a hard-coded path; rename/move would silently break until CI, consider centralizing path
    The drift guard that compares src/cockpit/web/lib/credentials-api.ts's MAX_STATUS_LENGTH to the domain MAX_STATUS_LENGTH reads a hard-coded repo path built from PROVIDERS_DIR. This is intentional per the comment, but it does mean a future file move/rename will break the test until CI. If we want to keep the mirror without brittle path coupling, consider exporting the mirrored constant from a tiny src/cockpit/web/lib/constants.ts and importing it into credentials-api.ts (and the test reads that file instead) — still no domain import in the web project, and the guard targets a single-purpose file less likely to move. Not blocking; the current approach is clear and deliberate.

Spec verification

Criterion Status Evidence
SC1. The providers table's Detail column renders a STATE, not an event or an instruction, for every provider; mechanically, at 1440x1000 no row truncates (≤46 chars working budget). Met src/cockpit/web/lib/credentials-api.ts defines MAX_STATUS_LENGTH = 40 and credentialStatusLine(listing) which prefers lastValidationStatus, skips presence-only and unconfigured rows, then uses a short detail (<=40) else derives a state ("validated"/"configured, never checked"). src/cockpit/web/widgets/Credentials.tsx renders credentialStatusLine(listing) in the Detail column instead of lastValidationDetail, ensuring a state-sized value. The 40-char cap is stricter than the ≤46 mechanical budget, preventing truncation.
SC2. The fix is cross-provider, not special-casing claude-code-token. A provider added later gets correct column behaviour without widget edits. Met Cross-provider enforcement is structural: the widget reads credentialStatusLine (generic). Additionally, packages/domain/src/credentials/providers/status-line.test.ts sweeps all provider source files and asserts every static status literal is ≤ MAX_STATUS_LENGTH and sentence-shaped strings are rejected (no trailing period). This prevents any one provider from reintroducing overlong status.
SC3. The transient post-action feedback surface keeps rendering the provider's full explanatory sentence, untruncated. Met src/cockpit/web/widgets/Credentials.tsx still renders <CredentialValidationResult result={validateResult} .../> for both Validate and Add success paths, which consume result.detail. No truncation is applied there. The column uses credentialStatusLine separately, leaving the transient surface unchanged.
SC4. A configured provider with no stored detail renders a distinguishable state (e.g. "configured, never checked"). Met credentialStatusLine in src/cockpit/web/lib/credentials-api.ts returns "configured, never checked" when listing.configured is true, no lastValidationStatus, and lastValidatedAt is absent or lastValidationDetail is over budget. This distinguishes "never checked" from an empty cell.
SC5. Rows written by the OLD shape still render correctly; back-compat posture is explicit. Met credentialStatusLine includes a back-compat rung: if lastValidationStatus is absent but lastValidationDetail exists and detail.length <= MAX_STATUS_LENGTH, it returns the detail. Otherwise it derives a state. This is documented in the function’s docblock and enforced in code.
SC6. If the full detail is no longer shown inline, it remains reachable (e.g., title tooltip). Met src/cockpit/web/widgets/Credentials.tsx CredentialRow renders the state line with title={listing.lastValidationDetail}. This keeps the original full sentence discoverable via tooltip while the cell shows only the short state.
SC7. The ask-side listPresence() consumer is explicitly accounted for; it should carry the right field and why. Met packages/domain/src/credentials/request-resolver.ts createCredentialRequestResolverDeps().listPresence() maps the credential listing and deliberately carries lastValidationDetail (not lastValidationStatus), with an in-code rationale explaining that presence payloads are not width-constrained and the fuller explanation is preferable for resolving asks.

Adoption sweep

Symbol Kind Consumers found Classification Notes
MAX_STATUS_LENGTH (packages/domain/src/credentials/index.ts export) function src/cockpit/web/lib/credentials-api.ts — mirrored constant (not imported by design), packages/domain/src/credentials/providers/status-line.test.ts — imports MAX_STATUS_LENGTH for assertions Adopted This is a re-export of a constant for domain consumers; the cockpit mirrors the value by literal per design. Test in providers/status-line.test.ts reads it and also guards the cockpit mirror against drift by parsing the web file.

Documentation impact

  • no-update-needed — Behavioral change is internal to the cockpit providers table rendering and API response shape; docs/ carries no references to /api/credentials shapes per the task spec’s verified sweep. No CLI or public API contract changed beyond adding an optional status field in JSON responses that remains backward compatible. No existing docs assert the old Detail-column behavior, so nothing is invalidated.

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Re-verification complete for mt#5031’s fix. Prior blocking concerns were addressed: the Detail column now renders a state via credentialStatusLine, providers supply bounded status values, and the server’s validate route allowlists status. Back-compat is explicit and safe; presence-only and unconfigured rows are handled; the ask-side listPresence() intentionally keeps the full detail. Tests cover rule precedence, class invariants, and component wiring, including negative controls. I found no new blocking defects; one minor nit about passing possibly-undefined title is non-blocking. Overall, the fix is correct, cross-provider, and well-tested. APPROVE.

Findings

  • [NON-BLOCKING] src/cockpit/web/widgets/Credentials.tsx:329 — Optional title may be undefined when no lastValidationDetail exists
    <span ... title={listing.lastValidationDetail}> sets the title to a possibly undefined value when no prior detail exists (e.g., configured-never-checked). Browsers generally ignore title attributes that are empty/undefined, so this is benign; calling title={listing.lastValidationDetail ?? undefined} (current shape) is equivalent. Consider explicitly using undefined or an empty string to make the intent clear, but this is non-blocking.

Spec verification

Criterion Status Evidence
SC1. The providers table's Detail column renders a STATE, not an event or an instruction, for every provider — mechanically, at 1440x1000 no row truncates (≤46 chars working budget). Met src/cockpit/web/lib/credentials-api.ts:96-145 implements credentialStatusLine() that always returns a short state-sized string or null and never the full lastValidationDetail. It enforces a 40-char cap via MAX_STATUS_LENGTH (src/cockpit/web/lib/credentials-api.ts:86-94), stricter than ≤46, preventing truncation. src/cockpit/web/widgets/Credentials.tsx:325-337 renders this status in the Detail column, not lastValidationDetail. Providers now supply status (e.g., packages/domain/src/credentials/providers/telegram.ts:118-124, github.ts:79-88) ensuring state-shaped values.
SC2. The fix is cross-provider: not special-casing claude-code-token. A provider added later gets correct column behaviour without widget edits. Met The widget calls generic credentialStatusLine(listing) (src/cockpit/web/lib/credentials-api.ts:96-145) regardless of provider. All existing providers set bounded status where applicable (e.g., anthropic.ts:55-60, google.ts:36-41, supabase.ts:48-53, railway.ts:53-58, telegram.ts:118-124, claude-code-token.ts:200-203, github.ts:79-91). The class invariant test packages/domain/src/credentials/providers/status-line.test.ts sweeps all providers and asserts status literals ≤ MAX_STATUS_LENGTH, preventing reintroduction.
SC3. The transient post-action feedback surface keeps rendering the provider's full explanatory sentence, untruncated. Met src/cockpit/web/widgets/Credentials.tsx:305-323 still renders <CredentialValidationResult result={validateResult} .../> after Validate/Add, which uses result.detail. The column separately uses credentialStatusLine. The component test src/cockpit/web/widgets/Credentials.test.tsx:420-489 asserts the full long sentence appears in the transient surface.
SC4. A configured provider with no stored detail renders a distinguishable state rather than an empty cell (e.g., "configured, never checked"). Met credentialStatusLine() returns "configured, never checked" when configured with no lastValidatedAt or long/absent detail (src/cockpit/web/lib/credentials-api.ts:137-145). Component test covers this case (src/cockpit/web/widgets/Credentials.test.tsx:568-575).
SC5. Rows written by the OLD shape still render correctly; the migration/back-compat posture is explicit in code. Met Back-compat rung in credentialStatusLine() passes through a short pre-split lastValidationDetail (≤ MAX_STATUS_LENGTH) (src/cockpit/web/lib/credentials-api.ts:132-140). The domain persists optional lastValidationStatus without rewriting old rows; comments document posture (packages/domain/src/credentials/lifecycle.ts:44-59). Tests exercise pre-split long details falling back to derived states (src/cockpit/web/lib/credentials-api.test.ts:115-133).
SC6. If the full detail is no longer shown inline in the column, it remains reachable (e.g., via a title tooltip), so truncation never destroys information. Met src/cockpit/web/widgets/Credentials.tsx:327-336 sets title={listing.lastValidationDetail} on the rendered status line. Component test verifies tooltip carries the full long detail (src/cockpit/web/widgets/Credentials.test.tsx:591-604).
SC7. The ask-side listPresence() consumer is explicitly accounted for: state which of the two fields it should carry and why, in code. Met packages/domain/src/credentials/request-resolver.ts:170-188 keeps carrying lastValidationDetail into presence payloads with an explanatory comment that width constraints do not apply there and the fuller explanation is preferable.

Adoption sweep

Symbol Kind Consumers found Classification Notes
MAX_STATUS_LENGTH function packages/domain/src/credentials/index.ts — re-exports MAX_STATUS_LENGTH, packages/domain/src/credentials/providers/status-line.test.ts — imports for assertions, src/cockpit/web/lib/credentials-api.ts — mirrored literal value guarded by test Adopted Cockpit mirrors the constant by value per design; a dedicated test parses the mirror to prevent drift. No additional wiring required.

Documentation impact

  • no-update-needed — Behavior change is limited to cockpit UI rendering and an optional status field added to the existing JSON response shape for credentials/validate and list. The spec and PR confirm docs/ has no references to /api/credentials or lastValidationDetail/status, and no CLI behavior changed. No existing docs assert the old Detail-column behavior, so nothing is invalidated.

@edobry
edobry merged commit 30d715f into main Sep 8, 2026
22 checks passed
@edobry
edobry deleted the task/mt-5031 branch September 8, 2026 06:22

@minsky-reviewer minsky-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: 2


Re-verification against the current diff shows the structural split is correctly implemented: providers now supply bounded status values, the Detail column renders credentialStatusLine() (never the full detail), and the server’s validate route explicitly allowlists status. Back-compat and presence/unconfigured cases are handled, and listPresence() intentionally keeps carrying the fuller detail. Tests cover provider invariants, the rule’s precedence, component wiring, and a drift guard for the mirrored constant. I find no new blocking defects introduced by these changes. Verdict: APPROVE.

Spec verification

Criterion Status Evidence
SC1. The providers table's Detail column renders a STATE, not an event or an instruction, for every provider — mechanically, at 1440x1000 no row truncates (≤46 chars working budget). Met src/cockpit/web/lib/credentials-api.ts defines MAX_STATUS_LENGTH = 40 and exports credentialStatusLine(listing) which always returns a short state or null and never the full lastValidationDetail. src/cockpit/web/widgets/Credentials.tsx renders credentialStatusLine(listing) in the Detail column instead of lastValidationDetail. Providers set bounded status values (e.g., packages/domain/src/credentials/providers/*.ts updates), and the 40-char cap is stricter than ≤46, preventing truncation.
SC2. The fix is cross-provider: it does not special‑case claude-code-token. A provider added later gets correct column behaviour without editing the widget. Met The widget calls generic credentialStatusLine() (src/cockpit/web/lib/credentials-api.ts) regardless of provider. All eight providers now supply a status where appropriate (changes in anthropic.ts, claude-code-token.ts, github.ts, google.ts, railway.ts, supabase.ts, supabase-service-role.ts, telegram.ts). A class-invariant test (packages/domain/src/credentials/providers/status-line.test.ts) sweeps all providers and asserts status literals ≤ MAX_STATUS_LENGTH.
SC3. The transient post‑action feedback surface (CredentialValidationResult) keeps rendering the provider's full explanatory sentence, untruncated. Met src/cockpit/web/widgets/Credentials.tsx continues to render <CredentialValidationResult result={validateResult} .../> for Validate/Add, which uses result.detail. The column separately uses credentialStatusLine, leaving transient feedback unchanged. Tests in src/cockpit/web/lib/credentials-api.test.ts and src/cockpit/web/widgets/Credentials.test.tsx assert the long sentence still appears in full on the transient surface.
SC4. A configured provider with no stored detail renders a distinguishable state rather than an empty cell (e.g., "configured, never checked"). Met credentialStatusLine() returns "configured, never checked" when configured with no lastValidatedAt or when detail is absent/over-budget (src/cockpit/web/lib/credentials-api.ts). Component test in src/cockpit/web/widgets/Credentials.test.tsx covers this rendering.
SC5. Rows written by the OLD shape still render correctly; the migration/back‑compat posture is explicit in code. Met Back‑compat rung in credentialStatusLine() passes through a short pre‑split lastValidationDetail (≤ MAX_STATUS_LENGTH) and otherwise derives a state (src/cockpit/web/lib/credentials-api.ts). Domain persists optional lastValidationStatus without rewriting old rows; comments document posture in packages/domain/src/credentials/lifecycle.ts. Tests in src/cockpit/web/lib/credentials-api.test.ts cover pre‑split long details falling back to derived states.
SC6. If the full detail is no longer shown inline in the column, it remains reachable (e.g., via a title tooltip), so truncation never destroys information. Met src/cockpit/web/widgets/Credentials.tsx sets title={listing.lastValidationDetail} on the rendered status line. Component test in src/cockpit/web/widgets/Credentials.test.tsx asserts the tooltip carries the full long detail.
SC7. The ask‑side listPresence() consumer is explicitly accounted for: state which of the two fields it should carry and why, in the code. Met packages/domain/src/credentials/request-resolver.ts keeps carrying lastValidationDetail into presence payloads, with an explanatory comment that width constraints do not apply there and the fuller explanation is preferable (lines added in the listPresence() mapping).

Adoption sweep

Symbol Kind Consumers found Classification Notes
packages/domain/src/credentials/MAX_STATUS_LENGTH function packages/domain/src/credentials/providers/status-line.test.ts — imports MAX_STATUS_LENGTH for assertions, packages/domain/src/credentials/index.ts — re-exports MAX_STATUS_LENGTH for domain consumers Adopted Cockpit mirrors the constant by value per design (src/cockpit/web/lib/credentials-api.ts) and a test guards against drift by parsing the mirror. No additional wiring required.

Documentation impact

  • no-update-needed — Change is internal to cockpit UI rendering and adds an optional status field to existing credential JSON payloads. The task spec verified docs/ contains no references to /api/credentials or these fields, and no CLI or user-facing documented behavior is changed or removed. I spot-checked for references in docs as per spec and found none to invalidate or augment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authorship/co-authored Co-authored by human and AI agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant