feat(server): widen update_bug_fields per the update-field audit - #40
Merged
Conversation
Expose the remaining agent-relevant PUT /rest/bug parameters as named
params on update_bug_fields, all under the existing fields capability:
summary, url, whiteboard, version, target_milestone, and add/remove
lists for keywords and see_also (bug URLs). Keywords and see_also
travel as {"add": [..], "remove": [..]} — never the replace-all set
variant, which would let a stale view wipe concurrent additions.
A see_also entry that points at this instance is a bug-id link, so the
tool assesses every local target the way update_bug_dependencies
assesses dependency targets (I8/I14): at least Capability::Summary,
uniform denial (I2), no PUT on refusal. Guard::see_also_local_id — the
parse the read paths already scrub links with — is public now so the
write side reuses it; foreign-tracker entries carry no local id and
pass through unassessed. Without this, a see_also write naming a
policy-denied bug would both record the reciprocal link on the hidden
bug and leak its existence through Bugzilla's success vs "does not
exist" answers.
Semantics stay deliberately narrow: empty strings and empty lists are
ignored (clearing a field is unsupported), one bug per call, and the
cf_ prefix gate (I7) is unchanged — see_also as a custom_fields key
still errors before Bugzilla is contacted. The at-least-one-field
check counts the new params. Free-text values (summary/whiteboard/url)
never enter the server log; the tool-entry trace records presence and
counts only, and the audit params allowlist keeps them (and the
see_also URL lists) at _len while adding keywords_add/keywords_remove
and target_milestone by value — closed instance vocabulary, the same
class as the already-allowlisted keywords and version.
DESIGN.md gains the full update-field audit table — every PUT
parameter either exposed via a named tool/param or withheld with its
reason, cc.remove included — and the matcher-visibility record stated
precisely: a write can never lift a bug out of a deny (a deny grants
nothing), but a restrict rule granting fields while matching on a
field fields can write is self-defeating, and a write CAN push a bug
into a denial, irreversibly through this server. examples/policy.toml
restates the operator warning next to the restrict examples.
Eight wiremock tests pin the wire shapes, the empty-value filtering,
the guard denial on new-fields-only calls, the denied see_also target
(uniform denial, zero PUTs, foreign entries never assessed), the I7
gate, and the all-empty refusal.
Closes #38
plusky
added
enhancement
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #38.
Implements the audit posted on the issue: every
PUT /rest/bugparameter is now either exposed through a named tool param or withheld with its reason recorded in DESIGN.md — the split is a decision, not an accident of tool growth.What changed
update_bug_fieldsgainssummary,url,whiteboard,version,target_milestone,keywords_add/keywords_remove, andsee_also_add/see_also_remove(bug URLs), all under the existingfieldscapability. Add/remove objects are built the wayupdate_bug_dependenciesbuilds them; the replace-allsetvariant is unreachable by construction.see_alsotargets are guard-assessed before the PUT: local-instance URLs are resolved to bug ids (Guard::see_also_local_id, the same parser the read paths use for scrubbing) and each target must passCapability::Summary— the I14/I8/I11 barupdate_bug_dependenciesandmark_as_duplicatealready apply — so a see_also write can neither link into a policy-denied bug (Bugzilla records a reciprocal entry on the target) nor be used as an existence oracle over denied ids (I2). Non-local URLs pass through untouched.cf_gate (I7) is untouched and now pinned by a test provingsee_alsocannot be smuggled throughcustom_fields; one bug per call stays.PARAM_ALLOWLISTrecordskeywords_add/keywords_remove/target_milestoneby value (closed instance vocabulary) while the free-text params stay length-only.Capability::Fieldsrustdoc, andexamples/policy.tomlupdated in step.Adversarial review record (pre-PR gate)
Three hostile lenses plus mutation verification against the implementation commit; 8 findings, all addressed, none rebutted:
update_fields_see_also_targets_respect_the_guard(uniform denial, zero PUTs). Since this fix landed after the workflow's mutation pass, I re-verified it by hand: neutering the target extraction makes the pin test fail.comment_is_privaterow had been reworded into claiming existing-comment privacy is handled elsewhere (it is not — it is withheld); restored to the plan's new-comment-privacy meaning.deny(grants nothing) but not for arestrictgrant that includesfieldswhile withholdingread: such a grant can be escaped in one write along a matched axis. The DESIGN.md record now states the bound precisely, including that arestrictrule grantingfieldson a matcher-visible axis is an escape hatch the operator must not build.keywords_*/target_milestoneadded to the audit param allowlist; the irreversibility direction (a write can push a bug into a denial that then makes the change unrevertable through this server) recorded in DESIGN.md and the shipped policy's comments;cc.removegiven its withheld row (completeness claim now true); "comment on every write tool" corrected (add_cc_to_bug takes none); the stalecomment_is_privaterationale resolved with the blocking docs finding.Mutation verification: 8/8 killed, no survivors (each applied singly in a detached worktree, killed by the named test, reverted): keywords-as-
set, flat see_also array, empty-string passthrough, guard-skip for new-fields-only payloads, remove-side dropped,cf_gate weakened, at-least-one-field miscount, comment dropped — plus the manual post-fix mutation above.Full AGENTS.md gate re-run independently in the isolated worktree: fmt, clippy
-D warnings,cargo test --workspace --all-targets --locked(294 tests),cargo deny check, typos — all green. No dependency changes.