Skip to content

feat(workspace): the role card's objectives in plain words — owned first, supported folded, no metric code names (abilityai/trinity-enterprise#843) - #3407

Open
dolho wants to merge 1 commit into
devfrom
feature/ent843-role-card-plain-objectives
Open

dolho wants to merge 1 commit into
devfrom
feature/ent843-role-card-plain-objectives

Conversation

@dolho

@dolho dolho commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Summary

The Workspace role card's Objectives now answer a client's question, "what is this agent for, and is it on track?", in plain words. Before, it showed canon objectives as written: metric code names (finance_ar_overdue_usd), "supports · quarter", "— / 1", and the same yellow "another agent measures this one" warning under every metric of every supported objective (nine times on one card in the operator's 2026-10-08 test).

  • Owned first, under "What this agent is responsible for". Each metric is one line: "Runway: 7.2 months (target 12 months) · updated 2h ago", or "not measured yet". A stale number drops the time and shows not updated recently.

  • Supported objectives sit in a separate group, collapsed by default with a count ("Also contributes to · N"), headings only: no metric rows and no per-metric warning.

    • "Tracked by " appears once per objective, in neutral text. The agent is named only when one agent this one holds a read grant on (served_by) serves every number; otherwise it says "Tracked by another agent".
    • An agent that only supports objectives still shows the folded group.
  • Readable metric names. role_card.fill_metric_labels takes, in order:

    1. the declared label;
    2. the label every active declaration of that name agrees on (db.agreed_metric_labels), so a metric another agent tracks shows by that agent's label;
    3. the code name in words (runway_months → "Runway (months)", finance_ar_overdue_usd → "Finance AR overdue (USD)").

    A raw snake_case name never reaches the card, and a failed lookup still falls back to words.

  • Warning colour only for real problems: stale, behind or off target, an unread file, an incomplete list. Remaining findings are neutral notes; "not measured" and "tracked elsewhere" aren't repeated under a metric.

  • Horizons read "this quarter", "this month" and so on.

  • client_heading: an optional plain heading for client screens on a canon objective, parsed by the join and used by the card, falling back to statement. The convention half (documenting the field in the canon objective format) is in trinity-pm.

  • Unchanged: the operator agent page keeps the full canon detail. The objective ↔ metric join (fix(monitoring): degrade fleet-status to 'unknown' on NULL status row (#669) #676) logic and honest status (principle 15) are as before.

Disclosure

PortalRoleMetric gains label. PortalRoleObjective gains client_heading, tracked_elsewhere and tracked_by. The literal key-set pins in test_ent676_role_card_join.py are updated with a note. tracked_by names only an agent this one already reads from under a grant. agreed_metric_labels returns labels only, never which agent declared them.

Tests

Not yet done

A visual check in the browser, light and dark (contract principle 29). The local instance is set up for today's demo, so I'll do it after.

Fixes abilityai/trinity-enterprise#843

🤖 Generated with Claude Code

…rst, supported folded, no metric code names (trinity-enterprise#843)

The Workspace role card showed canon objectives exactly as written: metric
code names, "supports · quarter", "— / 1", and the same yellow "another
agent measures this one" warning under every metric of every supported
objective.

- Owned objectives first, under "What this agent is responsible for", each
  metric as one line: "Runway: 7.2 months (target 12 months) · updated 2h
  ago", or "not measured yet".
- Supported objectives in a separate group, collapsed by default with a
  count, headings only; "Tracked by <agent>" once per objective (named
  only when one served agent holds every number), never per metric.
- Every metric crosses with a readable label: declared, else the label all
  active declarations agree on (db.agreed_metric_labels — labels only),
  else the code name in words. A raw snake_case name never reaches the card.
- Warning colour only for stale, behind/off target and unread files;
  remaining findings are neutral notes. Horizons read "this quarter".
- Canon objectives may carry an optional client_heading, used by the card
  and passed through the join.
- The operator view is unchanged; honest status (principle 15) stays.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dolho dolho added the ui PR touches the frontend UI — triggers Playwright e2e tests label Oct 8, 2026
@dolho

dolho commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author
0-before-after-light

@dolho
dolho requested a review from vybe October 8, 2026 14:46
@vybe

vybe commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

merge-train: ejected from this batch — rides the next train once fixed. CI is green and the tests do execute the changed path; these need a decision, not a mechanical fix.

1. "Tracked by " is unreachable in production. build_role_card calls objective_join_service.read_objective_join(agent_name, template=template, client=client) (src/backend/client_portal/role_card.py:450) without can_view, and resolve_served_metrics returns {} when can_view is None (src/backend/services/objective_join_service.py:1026). So no row carries served_by, tracked_by (role_card.py:242) is always None, and every supported objective reads "Tracked by another agent". test_ent676_role_card_join.py::test_the_card_never_resolves_another_agents_metric already pins "served_by" not in row. The Tracked by ${o.tracked_by} branch in PortalAgentRole.vue, the spec asserting "Tracked by finance-agent" (a hand-built row), and the new docs/memory/requirements/core-agent.md text describe behaviour the card cannot produce. It fails closed, so nothing leaks. Either pass a portal-principal can_view (which is a decision to name another agent to an external client) or drop the field, the branch and the claim.

2. agreed_labels reads labels fleet-wide. src/backend/db/metric_definitions.py:110 filters on name and active status only, with no owner or grant scoping. When the card's own agent does not declare the metric, the label an external client sees is authored entirely by other agents; on a multi-owner install any agent declaring the same metric name puts up to 120 characters on another owner's client-facing card. Vue escapes it, so this is a content/disclosure question, not XSS. Scoping to the same owner, or to agents the card's agent holds a grant on, would close it.

Smaller, for the same push if you agree:

  • New text (section heading, horizon line, toggle, "Tracked by" note, finding sentences) uses text-gray-400, which the design-system contract says is not a text colour. It matches the file's existing pattern, so the ratchet does not grow.
  • findingText's metric_undeclared / metric_not_declared_here strings are unreachable now that findingLine filters both codes.
  • declared_elsewhere = (not definition) and (not owned) is also true when no agent declares the metric, so "Tracked by another agent" can be false.

After merge, status-in-dev on abilityai/trinity-enterprise#843 is set by hand (cross-tracker keyword).

@vybe vybe added the status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) label Oct 8, 2026
vybe added a commit that referenced this pull request Oct 9, 2026
…orable secret value is a named 422 (#3325) (#3417)

Fixes #3325 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.**

## What
- `src/backend/services/credential_encryption.py::decrypt`: every malformed envelope shape raises `ValueError`, as the function promises — JSON that is not an object (`[]`, `null`), and a non-string nonce or ciphertext. Importing such a `.credentials.enc` now returns **400** instead of a 500 with a stack trace in the log. The envelope format and every successful decrypt are unchanged.
- `src/backend/services/secret_settings.py::encrypt_secret_setting`: the key is checked first, in its own `try`, and only that check can raise `MissingEncryptionKeyError`. A value that cannot be encoded (for example a lone surrogate) raises the new `SecretSettingValueError(ValueError)`. Its message names the setting, never the value.
- `src/backend/error_handlers.py` + `src/backend/main.py`: one app-level handler maps `SecretSettingValueError` to **422**. `src/backend/routers/settings/credentials.py`: three pass-through lines in the routes whose catch-all would have turned it into a 500 (Anthropic, GitHub PAT, `PUT /slack`).
- Tests: the three strict-xfail markers in `test_ec_credential_crypto_edges.py` are removed; new `tests/unit/test_3325_credential_error_types.py` (registered in `tests/registry.json`) asserts no value in the message or the exception chain, a missing key reported before a bad value, `PUT /api/settings/api-keys/anthropic` → 422, and that `main.py` registers the handler.

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- TD-1: one app-level handler plus the three pass-through lines — as recommended.
- TD-2: a `.credentials.enc` that decrypts correctly but whose contents are not an object is deferred — as recommended; forging one needs the platform key.
- TD-3: 422, matching the existing 422 for refused cleartext writes — as recommended.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, no critical findings. The secret cannot leave through the new error: it is raised after the `except` block, so `__context__` is cleared, not merely hidden; the handler returns only the message; nothing logs it; audit rows are written only after a successful write. The only residual carrier is traceback frame locals, which nothing in the backend renders. The handler is keyed on the subclass, so no other `ValueError` is swallowed or relabelled. `/cso --diff`: no findings.

Evidence sweep (claude-opus-5-5): **GREEN**. 23 unit files import the touched modules; the 16 most direct ran shuffled (seed 12345), one file per process, all passed. All 7 tests in the new file ran, 0 skipped. With both service files taken from `dev`, exactly the three formerly-xfail tests fail (8 cases). Seven less direct files are left to CI and named in the review file. `git merge-tree` against `dev` f6bedcd is clean.

## Tests
Targeted counts above. Not run here: the full unit island (CI).

## Before merge
- Startup auto-import (`lifecycle.py`) on a malformed envelope now lands in its `ValueError` retry arm: it retries without a warning per attempt and ends in the same "failed" result, with the existing error log after the last attempt.
- Enterprise callers of `decrypt` get `ValueError` where they got `AttributeError` / `TypeError` on a malformed envelope. Two catch broadly, three propagate as before; no change needed there.
- The new test file has a module-level `importorskip` for `sqlalchemy` and `cryptography`: it skips silently where those are missing. They are present on the engineer and in CI.
- Ten open PRs share `tests/registry.json` or `src/backend/main.py` with this branch (#3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`.

## Handoffs (not in this diff)
- TD-2 above: the decrypted-but-not-an-object case can still 500 on import.
- `PUT /api/settings/slack` saves the client ID before a later secret write fails (partial update, pre-existing).
- The GitHub PAT, Resend and Gemini routes are covered by the app-level handler but not exercised by the new route test.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
vybe added a commit that referenced this pull request Oct 9, 2026
…L is refused, not stored (#3323) (#3420)

Fixes #3323 — independent fix, one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. Base is `dev`. **Draft until the operator merges.**

## What
- `src/backend/utils/url_validation.py::reject_embedded_credentials`: strips its input and treats a leading `//` as already having a host, the same rule `strip_url_credentials` uses. `//<token>@github.com/o/r` is now refused like the `https://` form, including with leading whitespace or mixed case. All three write paths that store a skills-library URL go through this one function (`routers/skills.py` create and update, legacy adoption in `services/skill_service.py`), so the token is no longer saved in plaintext to `skill_sources.url` or the audit details.
- If `urlparse` refuses an input that starts with `//`, the function falls back to the module's existing userinfo regex instead of raising, so this fix adds no new 500.
- Tests: the D3 strict-xfail marker is removed; four more refused shapes and a four-case "must not raise / must not over-reach" test are added.

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- TD-1: the fallback applies only to inputs starting with `//`; the bare `ValueError` for every other malformed input is #3322's and is unchanged (D4 stays a strict xfail) — as recommended.
- TD-2: keep the username/password check; `//@host` carries no secret and is not refused — as recommended.
- TD-3, TD-4: deferred, see Handoffs — as recommended.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only): **MERGEABLE**, zero critical, five informational. The pre-fix and post-fix modules were executed side by side over forty inputs: nothing that was refused before now passes, credential-free inputs are unchanged, the fallback regex is anchored and linear. `/cso --diff`: no findings.

Fixed after review:
- `546f329a` — the `tests/registry.json` descriptions no longer call #3323 a strict xfail.
- `a75a5e04` — the reject stub in `test_ent183_skill_packages.py` mirrors the fixed `//` rule. It is deliberately stricter than production on a malformed bracket (`//[oops/x` raises).

Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 28 unit files import the module; the 14 most direct ran shuffled (seed 12345), one file per process. Three did not come back clean, none because of this branch:
- `test_ent236_skills_lifecycle.py::TestNonRepoDirectoryRecovers::test_real_repo_still_pulls` and `test_skill_service_user_agent.py` (collection error) fail the same way on `dev` f6bedcd in the engineer's environment.
- `test_ent237_skill_sources.py` hit the 240 s per-file limit at about 72 % with no failure printed — left to CI.

`git merge-tree` against `dev` f6bedcd is clean.

## Tests
895 passed across the 11 files that completed cleanly. Not run here: the 14 less direct importer files, `test_ent237_skill_sources.py` to completion, and the full unit island (CI).

## Before merge
- Ten open PRs share `tests/registry.json` with this branch (#3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2709). A test-merge of this head against each adds no conflict beyond what that PR already has with `dev`.
- One stale docstring remains at `tests/unit/test_ec_url_validation_properties.py:36` (still calls D3 a strict xfail).

## Handoffs (not in this diff)
- TD-3 (P3): shapes that still pass both reject and strip — `/\t/tok@`, `/\n/tok@`, `///tok@`, `https:/tok@` (`urlparse` removes tab and newline after the check). Fixing it needs the same change in `strip_url_credentials` and cases in both test families.
- TD-4: a property test that reject and strip always agree over the shared `test_2052` corpus; it needs exceptions for empty userinfo.
- #3322 (bare `ValueError` / `UnicodeError` → 500) is the sibling issue in the same file and is untouched.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
vybe added a commit that referenced this pull request Oct 9, 2026
…t the database (#3385) (#3421)

Fixes #3385 — one of ten small bug fixes in the trinity-pm `chain-easy-1008` run. **Stacked on #3414** (the fix for #3314): base is `feature/3314-queue-fingerprint-falsy`, because both issues edit `_clamp_ingested_item` and the fingerprint helpers. Merge #3414 first, then retarget this to `dev`. **Draft until the operator merges.**

## What
- `src/backend/services/operator_queue_service.py`: new `OPERATOR_QUEUE_TYPE_MAX = 64` and `_bounded_type`. An agent-written `type` longer than 64 characters is shortened with the existing `…[truncated]` marker at ingest (`_clamp_ingested_item`); the ask is never refused or lost. `_comparable_type` applies the same bound, so a shortened row does not read as a rewrite (#2915) and a row stored before this fix at full length still compares equal.
- `src/backend/db/operator_queue.py::_insert_values`: `_DB_BELT_TYPE_MAX_BYTES = 1024`; above it the insert raises `ValueError`, like the id / title / question / context belts. All three create paths go through it.
- Docs: five lines in `architecture/api-endpoints.md`, `feature-flows/operating-room.md` and `requirements/security.md` that list the clamp and belt fields now name `type`.
- Tests in `tests/unit/test_1632_operator_queue_caps.py`: an oversized type is shortened with the marker; every platform-emitted type and every type `ask_operator` accepts is unchanged; the belt rejects an oversized type and passes 64 four-byte characters; `test_3385_clamped_type_is_not_a_rewrite` (clamped row vs raw entry, legacy full-length row, a real rewrite inside the first 52 characters).

## Rulings carried (orchestrator, on the operator's behalf — plan file)
- T1: a fixed constant, no environment variable — as recommended.
- T2: 64 characters — as recommended. The longest type the platform emits is `workspace_problem_report`, 24.
- T3: the DB belt raises, like its siblings — as recommended.
- Orchestrator: stacked on #3314 instead of branching from `dev`, so the two fixes do not hand the operator a conflict in the same two functions.

## Review + security
`/review` reading pass (claude-fable-5-1, report-only, against the stacked base): **MERGEABLE**, zero critical. Census of every literal `type` the platform and the enterprise submodule pass to the insert paths: longest is 24 characters, none is altered. Every truncated value ends in the marker, which no set member can match, so truncation cannot move a value into or out of a set that decides control flow (`_BUDGETED_ALERT_TYPES` and neighbours). The new `ValueError` is reachable only from the file sync loop, which quarantines it under its existing handler. `/cso --diff`: no findings; the error text carries the constant, never the value.

Fixed after review: `eab91c10` — I1, the five doc lines. Noted, not changed: I2, a non-string `type` over 1 KiB now raises the named `ValueError` where the base failed at the bind — same quarantine outcome.

Evidence sweep after the fix (claude-opus-5-5): **GREEN**. 13 operator-queue files ran shuffled (seed 12345), one file per process: 836 passed, 7 xfailed. With both production files taken from the #3314 branch, the two `TestClampType` cases, the belt test and the clamped-row fingerprint case go red. `git merge-tree` against `dev` f6bedcd is clean; the base branch has not moved (`923b1edc`).

## Tests
Counts above. Not run here: `test_ent815_queue_walk`, `test_ent815_broad_list_agent_scope`, `test_ent751_gate_entries` (2–4 minutes each; they ran green on the #3314 base in that PR's sweep) and the full unit island.

## Before merge
- **This PR's checks are weaker than a dev-based PR's:** a PR whose base is a feature branch runs a reduced CI set and `backend-unit-test` does not run. The first full run happens when it retargets to `dev` after #3414 merges — read that run before merging.
- Accepted trade-off: an agent that rewrites an oversized type only after character 52 is not detected as a change. Titles and questions already behave this way.
- Thirteen open PRs share a file with this branch (mostly `tests/registry.json` and `docs/memory/architecture/api-endpoints.md`; #3256 also `db/operator_queue.py` and the three docs). A test-merge of this head against each of #3420, #3417, #3412, #3411, #3407, #3304, #3271, #3256, #2984, #2956, #2713 and #2709 adds no conflict beyond what that PR already has with `dev`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

@github-actions

github-actions Bot commented Oct 9, 2026

Copy link
Copy Markdown

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

@vybe

vybe commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

merge-train (2026-10-10): ejected — nothing has been pushed since the 2026-10-08 ejection, so both criticals stand: (1) tracked_by is unreachable in production (client_portal/role_card.py:450 calls read_objective_join without can_view, so served_by is never set and the Vue "Tracked by …" branch, the spec row and the requirements text describe output the card cannot produce — test_ent676_role_card_join.py:686 pins the opposite); (2) db/metric_definitions.py:250 agreed_labels is fleet-wide, never scoped by agent_name/owner. Also now conflicts with #3423 on dev: five files are mechanical, but PortalAgentRole.vue needs a decision on where the Guard badge lives once the supported-objective metric rows are gone. Rides the next train once fixed.

This branch has not been deployed

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

Labels

status-needs-fix PR has an unaddressed review/validation finding; cleared by the author's next push (#2815) ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants