fix: integrate structured Workdir refusal diagnostics
Merge independently approved T-731 source 8af492ff into current develop after T-732. No conflicts or source edits. Integration root locked/offline check, eight affected crates, Workdir default/http-client, Server 742+11 (one existing ignored), API schema freshness, fmt/diff passed. Historical Workdir403 remains unidentified; HTTP-only feature-gating limitation remains separate.
This commit is contained in:
@@ -0,0 +1,159 @@
|
||||
# Workdir refusal diagnostics
|
||||
|
||||
This change improves the internal `api_error` record for
|
||||
`POST /api/w/{workspace_id}/workers/self/workdir-session/operations`.
|
||||
It does **not** identify or resolve the reported 2026-10-09 403: the running
|
||||
binary revision and actual request are still unverified. Fixture success is
|
||||
not evidence about that incident. No deployment, live asset cleanup, privilege
|
||||
change, retry policy, or WIP dependency/version change is included (WIP remains
|
||||
0.2.0).
|
||||
|
||||
## Diagnostic path and public contract
|
||||
|
||||
Original refusing branches attach a closed `WorkdirDenialReason`, rather than
|
||||
recovering meaning from an error string. `WorkdirError::Denied` carries local
|
||||
message plus optional typed reason. `DenialContext` retains an existing error
|
||||
classification while enriching its diagnostics (notably checkout's historical
|
||||
permission-to-out-of-scope mapping). Typed provider markers survive the
|
||||
`FsAccessPolicy`/`io::Error` → `FsError` → `WorkdirError` boundaries.
|
||||
|
||||
`WorkdirTransportError.denial_reason` and
|
||||
`ExternalWorkdirOperationError.denial_reason` carry only an optional snake-case
|
||||
enum. Remote HTTP and External errors preserve that reason when converted back
|
||||
to `WorkdirError` and then forwarded again. Provider text is not the reason.
|
||||
The External codec continues dropping provider-authored message text. A missing
|
||||
reason stays unknown for generic `denied`; existing code-level path/symlink/
|
||||
read-only classifications can still identify their original category. Unknown
|
||||
or oversized reason strings fail deserialization rather than becoming log data.
|
||||
|
||||
Runtime registry session-open failures now retain a safe transport error **before**
|
||||
the former raw error-to-string conversion. Their existing Workspace HTTP meaning
|
||||
is still 502 (`workdir_session_open_failed`); operation-level permission failures
|
||||
remain 403. Session-open messages are normalized to the same safe provider
|
||||
message vocabulary instead of forwarding raw local errors. No authorization,
|
||||
scope, capability, session ownership, command ownership, or retry conditions
|
||||
are relaxed.
|
||||
|
||||
The self handler attaches internal context to its error. Both the handwritten
|
||||
response adapter and the generated route preserve it through the typed error
|
||||
response extension to `ApiErrorLog` and the logging middleware. Server API
|
||||
`workdir_log` and `denial_reason` are `serde(skip)` and omitted from schemas:
|
||||
the external error envelope remains `{code, message}` for provider failures or
|
||||
the existing Repository-style envelope for API/domain failures. Workspace
|
||||
responses do not expose the new internal diagnostics. Authorized Runtime/provider
|
||||
transport carries the enum to its consumer; it contains no free text or paths.
|
||||
|
||||
## Fields in `yoi::api` / `api_error`
|
||||
|
||||
- `operation`: fixed label from the typed operation (read, read_bytes, write,
|
||||
edit, command_start/status/output/cancel, authorize_scope, scope_rules_overlap,
|
||||
checkout_search/observe/execute, stat/list/glob/grep). No operation Debug dump.
|
||||
- `workdir_stage`: workspace_scope, worker_identity, attachment_validation,
|
||||
session_open, command_lookup, provider_dispatch, or command_registration. API
|
||||
failures identify the encountered stage without copying raw alias/handle/error
|
||||
text into the log. Registration failures can follow successful command start;
|
||||
they are not evidence of a pre-dispatch refusal or absence of effects.
|
||||
- `denial_reason`: optional closed reason reported by the provider/error chain.
|
||||
It describes the encountered refusing branch, not a permission grant and not
|
||||
an independent attestation of an external provider's explanation.
|
||||
- `workspace_id_hash`, `runtime_id_hash`, `worker_id_hash`, `workdir_id_hash`,
|
||||
`attachment_alias_hash`: 64-character lowercase SHA-256 fingerprints via
|
||||
`worker_source::diagnostic_id_hash`, using the existing
|
||||
`yoi-runtime-proof-diagnostic-id-v1\0` domain prefix. Hash a known catalog ID
|
||||
or attachment alias to locate its logs. Workspace and alias represent route/
|
||||
request selection; Runtime/Worker are added only after current identity checks,
|
||||
and Workdir only after a validated attachment is found. Missing information is
|
||||
omitted, never inferred from a failed lookup or command handle.
|
||||
- `request_token_id_hash`: fingerprint of the **existing verified Runtime proof
|
||||
JTI**, reusing the T-729 hash algorithm. It is not the proof, raw token ID,
|
||||
caller-supplied trace header, or a newly invented cross-service trace ID.
|
||||
In-process/test callers without that proof correlation omit it. Authentication
|
||||
still consumes the JTI using the existing policy; logging does not retry it.
|
||||
- Existing `method`, query-free `path`, `status`, `kind`, `message`, `diagnostics`
|
||||
remain. Workdir-context method/path are bounded to 32/512 UTF-8 bytes with
|
||||
controls removed. Provider operation messages remain normalized safe text.
|
||||
Contextual API failures use a fixed internal message and status-based kind,
|
||||
with no raw domain diagnostics copied into this log. Empty `diagnostics` does
|
||||
**not** mean the optional typed reason/context is absent.
|
||||
|
||||
Fingerprints are bounded correlation, not encryption: low-entropy identifiers
|
||||
can be guessed. Do not put credentials in IDs. Neither hashes nor stage labels
|
||||
prove the reported incident's cause. The request JTI identifies the incoming
|
||||
request; External operation IDs/generation/audit records remain their existing
|
||||
provider-side mechanisms, not a cross-service trace automatically joined here.
|
||||
|
||||
## Refusal vocabulary and limits
|
||||
|
||||
Reasons distinguish scope resolution/comparison unavailable, attenuated
|
||||
capability refusal, logical/parent/attachment/delegation scope exceeded,
|
||||
empty scope, invalid scoped path, writable-scope requirements for commands,
|
||||
child write-lease conflicts, unreadable cwd, read-only session, external root
|
||||
symlink/change/move, external scope symlink, checkout output-root escape,
|
||||
provider root escape, checkout search/enumeration refusal, provider symlink/
|
||||
checked-target refusal, path out of scope, symlink target out of scope, and
|
||||
read-only path. Original local messages remain local; arbitrary message text
|
||||
is never parsed into a reason.
|
||||
|
||||
- `os_permission_denied`: PermissionDenied with an actual OS error code, without
|
||||
a recognized policy marker. It does not determine whether modes, ACLs,
|
||||
sandboxing, mount policy, or another OS mechanism caused the refusal.
|
||||
- `unclassified_permission_denied`: custom PermissionDenied with neither typed
|
||||
policy origin nor raw OS code. Do not label an arbitrary
|
||||
`Error::new(PermissionDenied, "...")` as an OS refusal by guessing from text.
|
||||
- External/remote providers that do not send a reason cannot supply distinctions
|
||||
erased in their own binary. A registered provider's typed explanation is
|
||||
reported data, not proof of what happened inside that provider.
|
||||
- Authentication/JSON ingress failures before a typed Workdir operation and
|
||||
target can be obtained continue using their existing rejection diagnostics;
|
||||
no operation/body is guessed or decoded from rejected proof data.
|
||||
|
||||
## Regression proof boundaries
|
||||
|
||||
- Workdir provider fixtures exercise actual local authorization, broker scope/
|
||||
capability/read-only/delegation/lease refusals and confinement. Permission
|
||||
fixtures distinguish typed policy origins, OS-coded PermissionDenied, and
|
||||
origin-unknown custom errors, including checkout's existing public mapping.
|
||||
- Loopback HTTP fake exercises remote open/operation decoding and typed reason
|
||||
forwarding, including missing-reason providers and hostile provider text.
|
||||
External JSON/frame codecs exercise the bounded error envelope and reject
|
||||
arbitrary reason strings; the Server's External adapter drives the real self
|
||||
generated route through command-session dispatch and response/log conversion.
|
||||
- Server router tests use signed fixture requests and capture actual tracing
|
||||
events. They cover read/write/command_start/authorize_scope, session-open
|
||||
failures, External command status failures, no-identity rejection, oversized
|
||||
UTF-8 alias, oversized command/file/tool-call inputs, query/header/proof/key/
|
||||
credential markers, and raw provider/host paths. Identity hashes and the
|
||||
existing verified JTI fingerprint are checked against fixture identities.
|
||||
- The Server API boundary proves internal fields are absent from body/schema.
|
||||
Worker HTTP→WIP coverage verifies diagnostic wrappers do not change refusal
|
||||
classification or induce retries; tools retain established checkout codes.
|
||||
Normal-tool JSON→transport→WorkdirError→ToolsError→ToolError coverage checks all
|
||||
18 codes with no reason, a typed reason, and nested diagnostic wrappers. In
|
||||
particular, OutcomeUnknown/Conflict remain execution failures with unchanged
|
||||
display and inspection-before-retry guidance, not argument errors.
|
||||
Operational consumers share `WorkdirError::classification_source` to ignore
|
||||
only diagnostic wrappers while retaining the original error for diagnostics.
|
||||
Public Write tests cross JSON stat errors into the tool and the checked local
|
||||
provider: missing files are created identically without/with/nested reasons;
|
||||
denied/read-only/conflict/unknown-outcome stat failures never write or retry.
|
||||
This protects actual create control flow in addition to error conversion.
|
||||
- Real Remote HTTP and channel-backed External session dispatch tests cover
|
||||
Read/Create/Write/Edit crossed with missing/typed reasons and five error codes.
|
||||
After a dispatched mutation, Unavailable/Transport still become OutcomeUnknown
|
||||
with the original reason preserved: forwarded 500 and inspect-before-retry
|
||||
guidance are unchanged. Read and definite refusal/conflict controls retain
|
||||
their existing codes. The External path also checks actual HTTP error body/
|
||||
status conversion and absence of a new dispatch after the failure.
|
||||
- Fake-command JSON fixtures cover attachment detach and scoped-command close
|
||||
without launching processes. Missing/typed/nested UnknownCommand all retain
|
||||
disappearance/lease-release semantics. Unavailable remains a busy detach or
|
||||
failed scope cleanup; scope close revokes new work immediately but retains the
|
||||
write lease while cleanup is unresolved. Diagnostic data never grants cleanup.
|
||||
- A signed real self-handler test observes command dispatch, then removes only
|
||||
its fixture session-registration entry before returning the start handle.
|
||||
The 502 remains a post-start `command_registration` failure with a neutral
|
||||
safe log message; no process is launched and no retry is performed.
|
||||
|
||||
These are hermetic Rust/API/protocol fixtures, not a full product-process E2E,
|
||||
not a deployed binary test, and not reproduction of the user's asset/request.
|
||||
Validation commands and concrete counts are recorded on the implementation MR.
|
||||
@@ -0,0 +1,95 @@
|
||||
# T-731 independent review verdict registration rejected
|
||||
|
||||
## Confirmed observation
|
||||
|
||||
- Ticket: T-731, item revision `00001M4G6PHJQ:0`.
|
||||
- Selected MR: `01a120d6-36fb-7673-92ce-a4cd50ba0d7d`, repository `main`.
|
||||
- Selectors: `work/T-731-workdir-denial-diagnostics` → `develop`.
|
||||
- First reviewed source: `f9d70e2ae2c1b226fd5832dfa13102b9f1b6ac4c`.
|
||||
- Trusted parent-owned Reviewer binding recorded `ReviewRequested` at
|
||||
`2026-10-09T13:25:51.374292664+00:00`, MR seq2 event
|
||||
`01a120d7-6c98-7c91-bdc0-6c61d2fe66c1`.
|
||||
- Reviewer found a real normal-tool diagnostic-wrapper classification regression
|
||||
through a target-only public-API harness, after 30 focused tests, root check,
|
||||
formatting and diff checks passed.
|
||||
- The review-submission tool returned
|
||||
`Merge Request API returned HTTP 401` with no diagnostic response text. It did
|
||||
not record the intended request-changes verdict. The committed session's
|
||||
output reference is `E01a120e2-ae8b-7581-82f9-f0f43565c173`; its reproduction
|
||||
output is `E01a120de-c8a2-72d3-ba9b-6a9392d19dda`.
|
||||
- Parent `ShowMergeRequest` read at provider observation
|
||||
`2026-10-09T13:39:10+00:00` still showed only seq1 comment and seq2 request;
|
||||
there was no verdict/approval event. The tool response does not provide an
|
||||
exact failure timestamp, auth reason, or request correlation ID. None is
|
||||
inferred here.
|
||||
|
||||
## Handling and limitation
|
||||
|
||||
The first finding was fixed in forward commit `10c55874` on the same source ref.
|
||||
Source movement requires a fresh exact-source trusted review. The original narrative
|
||||
verdict is not substituted for a recorded authoritative review event.
|
||||
|
||||
Ticket seq4 records this operational blocker for Orchestrator attention. The
|
||||
Orchestrator directed continuation through the ordinary fresh trusted-review
|
||||
path after fix/test/publish, and stopping with bounded evidence if submission
|
||||
again returns 401. No independent Runtime Reviewer, alternate mutation endpoint,
|
||||
proof/auth/capability change, deployment update, or branch/MR replacement is used.
|
||||
|
||||
The HTTP 401 cause is unknown. It is not established to share a cause with the
|
||||
reported Workdir 403, and T-731's fixture diagnostics do not resolve either live
|
||||
incident. The registration failure is not repaired by the product-code changes
|
||||
in this Ticket.
|
||||
|
||||
## Fresh review: invalid severity rejected (422)
|
||||
|
||||
- Reviewed source after the first forward fix:
|
||||
`10c55874e3cc4c3562f608e10c56ff1c4c2a4d3c`.
|
||||
- Fresh request: MR seq3 event `01a120f4-6c90-7791-a36f-69d52e2cbd4b`,
|
||||
`2026-10-09T13:57:31.897275884+00:00`, same Ticket revision and sole MR.
|
||||
- Independent review found a second operational regression: normal Write's
|
||||
bare NotFound match failed to create files for typed/nested diagnostic context.
|
||||
A public-tool harness reproduced it despite 59 focused tests passing.
|
||||
- The sole executed verdict attempt returned HTTP 422, specifically invalid
|
||||
`findings[0].severity = medium`; Backend expects blocker/major/minor/note.
|
||||
Committed response: `E01a120fc-a7b7-79c3-b812-12cfd1038f75`.
|
||||
- Observed UTC bracket was 14:05:37.641361776Z–14:06:36.777627982Z on
|
||||
2026-10-09, not the exact server request timestamp. Parent MR reread at
|
||||
14:11:31Z still had no events after seq3.
|
||||
- Parent requested a schema-only correction on the unchanged snapshot, but the
|
||||
Reviewer role's exactly-once instruction disallowed a second verdict call.
|
||||
Its committed follow-up confirms no corrected submission executed. No other
|
||||
mutation path or Reviewer was used to bypass this restriction.
|
||||
|
||||
This 422 is input validation evidence, not proof that the earlier 401 has been
|
||||
resolved. The narrative request-changes judgment is not a recorded verdict.
|
||||
The reproduced second regression is addressed by the shared classification
|
||||
source and real Write regression in this forward change on the existing branch.
|
||||
The new test fails for the typed-reason case before the fix and passes all three
|
||||
forms after it. That source movement still needs fresh exact-source review, not
|
||||
reuse of the old review binding. The user-reported Workdir 403 remains unidentified.
|
||||
|
||||
## Later registration observation
|
||||
|
||||
A subsequent fresh binding on source
|
||||
`6ac87c885e58f11cb6dde0557c9229249f2c428e` successfully recorded request-changes:
|
||||
MR seq5 event `01a12114-771a-76b1-b0fb-2fde0d56e3e1` at
|
||||
`2026-10-09T14:32:31.769486531+00:00`, confirmed by parent MR reread. This proves
|
||||
that selected submission succeeded; it does not explain the earlier 401.
|
||||
No authentication, proof, capability, Backend, or running binary changes were
|
||||
made to obtain it. The remaining code finding and routine review evidence are
|
||||
on that MR, not an approval or Ticket conclusion in this report.
|
||||
|
||||
## Improvement proposal
|
||||
|
||||
A verdict-registration rejection should expose a safe, bounded internal
|
||||
operation/request-event correlation and classified auth failure (without proof,
|
||||
credentials, keys, request body, or raw provider errors), so the Orchestrator can
|
||||
locate the corresponding Backend record. The current bare 401 leaves the
|
||||
reviewer unable to distinguish authentication failure from downstream authority
|
||||
rejection. Investigate under separate authorization; do not weaken the guarded
|
||||
review path or retry blindly to hide the failure.
|
||||
|
||||
Also align the tool schema's severity vocabulary with the Backend enum, rather
|
||||
than advertising an unconstrained string that consumes an exactly-once verdict
|
||||
attempt on a deterministic 422. Any correction/review-grant lifecycle policy
|
||||
needs a separate explicit design; this Ticket does not change it.
|
||||
Reference in New Issue
Block a user