Skip to content

engine(gate): the screenshotTableGate action warning still names only close/advisory after the block tier shipped #9996

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

The screenshot-table gate accepts three action tiers. packages/loopover-engine/src/review/screenshot-table-gate.ts:36:

const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "block", "advisory"];

ScreenshotTableGateAction is "close" | "block" | "advisory"
(packages/loopover-engine/src/types/manifest-deps-types.ts:34) and the public OpenAPI schema agrees
(src/openapi/schemas.ts:962):

        action: z.enum(["close", "block", "advisory"]),

The operator-facing warning that fires when the configured value is not one of them was not updated when
the block tier landed (#9964, commit 3ed97dc70). packages/loopover-engine/src/review/screenshot-table-gate.ts:77:

  const action = isScreenshotTableGateAction(record.action)
    ? record.action
    : (() => {
        if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" or "advisory" (#4110 removed request_changes/comment as dead config surface); using the default "close".`);
        return DEFAULT_SCREENSHOT_TABLE_GATE.action;
      })();

This is the only remaining place in the repository that describes the action vocabulary as
"close" or "advisory" — a grep for that phrasing across src/, packages/ and *.md returns this line
and nothing else.

The practical effect: an operator who mistypes the value (blok, Block, hold) is told the accepted
values are "close" and "advisory", is silently defaulted to "close" — the destructive tier — and is
never told that block, the non-destructive tier #9964 added precisely so an enforcing gate holds a PR
instead of destroying it, exists at all.

Requirements

  • The warning text must name all three currently-valid actions, and must be DERIVED from VALID_ACTIONS
    rather than restating them as a literal, so a future tier addition cannot desynchronize the message again.
  • The (#4110 removed request_changes/comment as dead config surface) note must be preserved in the
    message: it explains why two historically-accepted values are now rejected, which is still the useful
    half of this warning for an operator upgrading an old config.
  • The message must still state which default is applied (DEFAULT_SCREENSHOT_TABLE_GATE.action), derived
    from the constant rather than hardcoded as the literal "close".
  • What must NOT change: VALID_ACTIONS, isScreenshotTableGateAction, DEFAULT_SCREENSHOT_TABLE_GATE, the
    condition under which the warning fires (record.action !== undefined and not a valid action — an ABSENT
    action must still produce no warning), and the resolved action value on every input. This is a warning
    text change only; no gate disposition changes.

⚠️ Required pattern: mirror the derived-vocabulary message already used by
normalizeSelfLoopAutonomy at packages/loopover-engine/src/ams-policy-spec.ts:179, which interpolates
AUTONOMY_LEVELS.join(", ") rather than restating the literals — its own comment
(packages/loopover-engine/src/ams-policy-spec.ts:176) says it is written that way specifically so it
"can't drift from the vocabulary the rest of the codebase resolves against". What does NOT satisfy this
issue: (a) editing the string to the literal "close", "block" or "advisory" — that is the same
drift-prone shape that produced this bug; (b) widening the change to every warning string in the file;
(c) adding a new fourth action tier; (d) a test-only PR.

Deliverables

  • packages/loopover-engine/src/review/screenshot-table-gate.ts — the warning pushed for an invalid
    action names close, block and advisory, interpolated from VALID_ACTIONS, and names the
    applied default from DEFAULT_SCREENSHOT_TABLE_GATE.action.
  • A regression test at packages/loopover-engine/test/screenshot-table-gate.test.ts (this file does not
    exist yet; create it, importing from ../dist/review/screenshot-table-gate.js per the convention in
    packages/loopover-engine/test/content-lane-flag.test.ts) named for this bug, asserting that
    normalizeScreenshotTableGateConfig({ enabled: true, action: "blok" }, warnings) pushes exactly one
    warning whose text contains the substring block, and that the resolved action is still "close".
  • The same test file asserts the no-warning path is unchanged:
    normalizeScreenshotTableGateConfig({ enabled: true }, warnings) pushes NO action warning, and
    normalizeScreenshotTableGateConfig({ enabled: true, action: "block" }, warnings) resolves to
    "block" with no warning.
  • Root-suite coverage at test/unit/ (an existing screenshot-table-gate suite if one covers this
    normalizer, otherwise a new file whose exact path the PR states) asserting the same three outcomes.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that fixes the string but only asserts the invalid-action path and never pins the absent-action no-warning
path — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's
coverage.include covers src/**/*.ts and packages/loopover-engine/src/**/*.ts — the touched path
packages/loopover-engine/src/review/screenshot-table-gate.ts IS measured.

Branches touched, each needing BOTH arms tested: the isScreenshotTableGateAction(record.action) ternary
(valid action → used as-is; invalid → default applied) and the inner record.action !== undefined guard
(present-but-invalid → warning pushed; absent → no warning). All four arms must be asserted.

Engine lines are credited by two uploads whose hits are unioned — add the test to
packages/loopover-engine/test/** as well as any root test/** coverage, or the patch gate can still fail.

Expected Outcome

An operator who mistypes requireScreenshotTable.action is told all three tiers the gate actually accepts,
including the non-destructive block tier, instead of being pointed at a two-value vocabulary that has been
out of date since #9964. The message is generated from VALID_ACTIONS, so adding a fourth tier updates it
automatically.

Links & Resources

  • packages/loopover-engine/src/review/screenshot-table-gate.ts:36VALID_ACTIONS
  • packages/loopover-engine/src/review/screenshot-table-gate.ts:77 — the stale warning text
  • packages/loopover-engine/src/types/manifest-deps-types.ts:34ScreenshotTableGateAction
  • src/openapi/schemas.ts:962 — the public schema, already carrying block
  • packages/loopover-engine/src/ams-policy-spec.ts:176 — the derived-vocabulary precedent to mirror
  • feat(gate): add the block tier, so an enforcing screenshot gate holds a PR instead of destroying it #9964 — the PR that added the block tier

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions