feat: new canister setting status_visibility - #745
Conversation
There was a problem hiding this comment.
Pull request overview
Adds end-to-end support for the management canister’s status_visibility setting.
Changes:
- Introduces shared visibility parsing, conversion, comparison, and formatting.
- Supports status visibility in manifests, creation, synchronization, updates, and status output.
- Updates dependencies, schemas, documentation, and integration coverage.
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
Cargo.toml |
Upgrades management canister types. |
Cargo.lock |
Locks upgraded dependencies. |
CHANGELOG.md |
Documents feature and compatibility impact. |
crates/icp/src/canister/mod.rs |
Adds status visibility to settings. |
crates/icp/src/canister/visibility.rs |
Implements shared visibility model. |
crates/icp/src/project.rs |
Resolves manifest status visibility. |
crates/icp/src/operations/settings.rs |
Synchronizes visibility settings. |
crates/icp-cli/src/commands/canister/create.rs |
Applies visibility during creation. |
crates/icp-cli/src/commands/canister/mod.rs |
Shares human-readable formatting. |
crates/icp-cli/src/commands/canister/status.rs |
Reports status visibility. |
crates/icp-cli/src/commands/canister/settings/show.rs |
Displays status visibility. |
crates/icp-cli/src/commands/canister/settings/update.rs |
Adds visibility update flags. |
crates/icp-cli/tests/canister_settings_tests.rs |
Adds replica integration coverage. |
docs/reference/canister-settings.md |
Documents access policies and forms. |
docs/reference/cli.md |
Documents new CLI flags. |
docs/reference/configuration.md |
Adds configuration example. |
docs/schemas/icp-yaml-schema.json |
Adds project schema support. |
docs/schemas/canister-yaml-schema.json |
Adds canister schema support. |
docs/schemas/environment-yaml-schema.json |
Adds environment schema support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
848ac32 to
e3e0ea1
Compare
e3e0ea1 to
1dccedc
Compare
lwshang
left a comment
There was a problem hiding this comment.
crates/icp-cli/tests/assets/__pycache__/limit_transfer.cpython-312.pyc looks unintentional — it isn't matched by .gitignore, so it'll show up as churn for anyone who runs the Python asset. git rm --cached it and add __pycache__/ to .gitignore.
Adds the `status_visibility` canister setting from dfinity/ic#10667, which controls who may read a canister's status through the management canister's `canister_status` endpoint. It takes the same three forms as `log_visibility` in a manifest and the same flag shapes on `icp canister settings update`; `icp canister status` and `icp canister settings show` report it, it is applied at creation, and it is kept in sync by `icp canister settings sync`. The three visibility settings the management canister exposes are structurally identical but distinct Candid types, so rather than duplicate the manifest type, the parser, the set-wise comparison, the add/remove merge and the renderer per setting, this introduces one shared `Visibility` enum in `icp::canister::visibility` plus a `visibility_setting!` macro that declares the per-setting manifest newtype, so parse errors and the generated schema still name the specific setting. `log_visibility` behaviour is unchanged. Requires `ic-management-canister-types` 0.9.0, which is where `StatusVisibility` landed. `DefiniteCanisterSettings` gained a non-optional `status_visibility`, so reading a canister's status now requires a replica that reports it — every mainnet subnet does, and so does the pinned network launcher. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QooMLZ15EWRrknWtXfFZNp
`Allowed viewers: <a>, <b>` put two 63-character principals on the label's
line, which wraps in any normal terminal. The policy now stays on the
label and each viewer gets its own line, nested two spaces past the label
the way the other lists in those reports are:
Status visibility: Allowed viewers
viewer: 7tjgl-udln4-...
viewer: cgbip-rubo2-...
The label sits at a different indent in `canister status` than in
`canister settings show`, so `format_visibility` takes the label's indent
and nests relative to it. `log_visibility` is rendered by the same
function, so it changes with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QooMLZ15EWRrknWtXfFZNp
Applies the per-line rendering to the controller list too, so the two
reports list principals the same way throughout:
Controllers:
controller: 2vxsx-fae
...
Log visibility: Allowed viewers
log viewer: aaaaa-aa
Status visibility: Allowed viewers
status viewer: 2vxsx-fae
`format_principal_list` holds the shared shape — sorted, one entry per
line, nested two spaces past the label, with a note where the entries
would have gone when the list is empty. Controllers were not sorted
before; they are now, since the replica does not promise an order.
Each entry names what it grants rather than a bare `viewer:`, because a
report carries a line for both visibility settings and the entries under
them would otherwise not say which they belong to.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QooMLZ15EWRrknWtXfFZNp
1dccedc to
733ae64
Compare
`crates/icp-cli/tests/assets/__pycache__/limit_transfer.cpython-312.pyc` was committed alongside the Python asset it is compiled from. Nothing reads it, and since `.gitignore` did not cover it, anyone who ran the asset saw it come back as a dirty working tree. Untracked, and `__pycache__/` ignored so it stays that way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QooMLZ15EWRrknWtXfFZNp
`--add-status-viewer P` on a canister whose status is `public` resolves to `AllowedViewers([P])`, which revokes everyone else's access rather than adding to it; `--remove-status-viewer P` on the same canister resolves to an empty list. The log flags behave the same way — the resolution is shared — and neither said anything about it. `resolve` stays pure. `maybe_warn_on_lost_access` runs at the callsite alongside `maybe_warn_on_env_vars_change` and warns in the two cases where a viewer edit costs access: the current policy is `public` and the result names viewers, or the result names none at all. A warning rather than a prompt, because unlike losing control of a canister either change can be undone by any controller, and a prompt would break scripted use. `--set-*-viewer` did not fetch the current settings, since it does not build on them, so the public-to-viewers case could not be detected for the one flag that states the new list outright. It now fetches too; the extra query buys a warning that fires for all three flags. Removal switches from `swap_remove` to `retain`, so the viewers it leaves behind keep their order — that order is what the `status viewer:` and `log viewer:` lines print. The resolution matrix is now unit-tested, which is what the integration test already claimed: a fixed policy beats viewer edits, `--set` replaces, `--add` dedupes, an edit against a fixed policy starts from an empty list, removal is order-preserving and may empty the list, and every viewer edit requires the current settings. The two warnings are covered against a live replica instead, since they go through `warn!`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QooMLZ15EWRrknWtXfFZNp
`public` grants access to everyone, so it carries no allowed viewers
list for `--add-log-viewer` / `--remove-log-viewer` and their status
counterparts to be relative to. They started one from empty instead,
which meant `--add-log-viewer P` on a public canister took access away
from everyone but P, and `--remove-log-viewer P` took it away from
everyone including P. Warning about it was the wrong tool: these flags
do not say to change the policy at all, so there is nothing to warn
about — the edit has no meaning to carry out.
They are now refused while the setting is public, and the error names
the two flags that do state a policy outright:
Log visibility is currently public, so there is no allowed viewers
list for --add-log-viewer to edit. Use `--set-log-viewer <PRINCIPAL>`
to replace the public policy with an explicit list, or
`--log-visibility controllers` to revoke public access on its own.
`controllers` keeps accepting relative edits, because a list built from
it grants access on top of the controllers rather than taking any away.
`--set-*-viewer` on a public canister stays legal for the same reason
the relative edits are not: it states the whole new list, so what it
revokes is on its face — it warns rather than refuses, as does removing
the last viewer.
`resolve` grows a `Result` and the group carries the setting's names, so
one implementation still serves both settings and its messages can name
whichever it was reached through.
This is breaking for the log flags, which shipped with the old
behaviour: `canister_settings_update_log_visibility` asserted the
add-to-a-public-canister case succeeding, and now asserts the refusal
followed by the `--set-log-viewer` that expresses it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QooMLZ15EWRrknWtXfFZNp
|
@lwshang — thanks, all four review points are addressed. Three commits on top of the stack:
I've replied on each thread but left them open for you to resolve. The PR description is updated to match: the |
What
Adds support for the
status_visibilitycanister setting introduced in dfinity/ic#10667, which controls who may read a canister's status through the management canister'scanister_statusendpoint — the running state, cycles balance, memory usage, and the settings themselves.It takes the same three forms as
log_visibility:and the same flag shapes on
icp canister settings update:The relative edits (
--add-*-viewer,--remove-*-viewer) are refused while the setting ispublic, which carries no allowed viewers list for them to be relative to. That rule is applied to the pre-existing log flags too, which is a behaviour change — see below.icp canister statusandicp canister settings shownow report it, in both their default and--jsonoutput. It is applied at creation and kept in sync byicp canister settings sync.How
The three visibility settings the management canister exposes (
log_visibility,snapshot_visibility,status_visibility) are structurally identical but are distinct Candid types. Rather than duplicate the manifest type, the parser, the set-wise comparison, the add/remove merge, and the renderer per setting, this introduces one sharedVisibilityenum inicp::canister::visibilitywith conversions to and from each Candid type, and a smallvisibility_setting!macro that declares the per-setting manifest newtype so parse errors and the generated JSON schema still name the specific setting.That replaces the hand-written
LogVisibilityDef(about 110 lines ofDeserialize/JsonSchemaboilerplate) and makes each further visibility setting a few lines. Downstream,log_visibility_eqbecamevisibility_eq, the foursettings updateflags share oneVisibilityFlags::resolve, andcanister status/settings showshare oneformat_visibility.VisibilityFlags::resolvestays pure — it neither reads the network nor prints — and carries the setting's names so its refusal and its warnings can say which setting they are about. The two warnings live at the callsite, alongside the existingmaybe_warn_on_env_vars_change.log_visibilitybehaviour is unchanged: same manifest forms, sameunknown log_visibility valueerror text, same$defs/LogVisibilityschema name, same rendering.Relative viewer edits on a
publiccanisterpublicgrants access to everyone, so it carries no allowed viewers list for--add-*-viewer/--remove-*-viewerto be relative to. They started one from empty instead, which meant--add-log-viewer Pon a public canister took access away from everyone butP, and--remove-log-viewer Ptook it away from everyone includingP. Neither flag says to change the policy at all, so there was nothing to warn about — the edit had no meaning to carry out.They are now refused while the setting is public, and the error names the two flags that do state a policy outright:
--add/--remove--set-*-viewer--*-visibilitypubliccontrollersallowed_viewers--set-*-viewerstays legal on a public canister for the same reason the relative edits are not: it states the whole new list, so what it revokes is on its face. It warns rather than refuses, as does removing the last viewer — both are reversible by any controller, and a prompt would break scripted use.This is breaking for
--add-log-viewer/--remove-log-viewer, which shipped with the old behaviour.canister_settings_update_log_visibilityasserted the add-to-a-public-canister case succeeding; it now asserts the refusal, followed by the--set-log-viewerthat expresses it. The status flags are new, so they were never able to behave otherwise.Adding a viewer to a
controllerscanister is unaffected: a list built fromcontrollersgrants access on top of the controllers rather than taking any away, so the edit means what it says.Dependency bump and replica compatibility
ic-management-canister-types0.8.0 → 0.9.0, which is whereStatusVisibilitylanded. That version also addsminimum_incoming_canister_call_cyclestoCanisterSettingsand requirescandid0.10.35 (hence the lockfile churn). Both new settings are sent asNone, leaving them unchanged.DefiniteCanisterSettingsgained a non-optionalstatus_visibility, so a CLI built against 0.9.0 requires a replica that returns it — otherwisecanister_statusfails to decode, which affectsicp deploy,icp canister status, andicp canister settings show/sync, not just the new setting.59c42492or79c01052).15.0.0-2026-08-20-03-30carries it, as does15.0.0-2026-08-13-03-55; anything older does not. Contributors with atarget/test-fixtureolder than that must re-runscripts/download_test_network_launcher.sh, or every status-reading test fails withfield status_visibility is not optional field.ic-utilsstill builds against 0.8, so itsCanisterSettingsis now a distinct type. The one call site that handed settings toic-utilswas the legacy management-canister fallback, which #744 already moved off it.Documented access policy
status_visibilityis not controller-only undercontrollers: subnet administrators always retain access, and a canister reading its own status is exempt.log_visibilityhas neither carve-out —check_log_visibility_permissionis a barehas_access(caller, controllers). The schema descriptions are therefore per-setting rather than shared, the reference documents the two always-allowed callers, andcanister settings showno longer claims the caller must be a controller (a non-controller granted status access can now run it).Testing
Unit tests for the shared pieces, which is where the logic now lives:
icp::canister::visibility— both manifest forms parse, round-trip through serialization, and convert to and from both Candid types; each setting names itself in its parse error.VisibilityFlags::resolve— fixed policy wins over viewer edits,--setreplaces,--add/--removeapply to the current list, adding a present viewer does not duplicate it, removal keeps the order of the viewers it leaves behind, an edit againstcontrollersstarts from empty, and only viewer edits require fetching the current settings.publicrefusal — each of--add,--remove, and both together, checking that the error names the flags that were passed and both ways out, and that--setis still allowed there.canister status --json— the tagged{"type","value"}shape is pinned, since that output goes through a hand-writtenSerializeimpl rather than a derive.Against a local network:
canister_settings_update_status_visibility(new) — every flag, checked both insettings showand in what it actually grants: a non-controller is refused and falls back to the public information, then reads the full status once added as a viewer or once the status is public, and is refused again when it is revoked. Also the refusal and the two warnings against a live replica, which is where they can be exercised end to end, and that an update naming neither visibility group leaves both untouched rather than resetting them.canister_settings_update_log_visibility(extended) — the refusal on a public canister, and the warning--set-log-viewerprints in its place. The rest of the test is unchanged.canister_settings_sync_log_visibility(extended) — a single sync drives both settings from the manifest, which is the case that matters: either change alone would satisfy the "settings already match" check that decides whether to send an update. The three manifest forms themselves are already covered there forlog_visibilityand go through the same shared parser, so they are not repeated per setting.canister_settings_show(extended) —--jsoncarries the new field.Not in this PR
snapshot_visibilityis still passed asNone. Its type has shipped since 0.8.0 and the existing TODOs called it out for a follow-up; with this shared machinery in place it is now a short one.The two commands that report settings disagree on JSON shape, and
settings show --jsonrenders numeric settings asNatlimb arrays. Both predate this PR and are filed as #728.