Add human actor identity descriptors - #224
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.98
The patch introduces an arbitrary-command authority bypass, two concrete cross-version blocking paths, and an unbound declarative approval path. Questions for model-level verification: can any other identity-bearing suspension resume after provider/configuration drift, and does every unsupported durable revision retain a safe migration or reauthorization path? Model-level verification is recommended before merge.
Findings without inline diff anchors
[P1] Prior delegation records permanently strand active runs
Invariant: obsolete authorization must fail closed while retaining a reachable explicit reauthorization path. After upgrading an active run containing a real schema-2 delegation record, Load rejects it; normal flow execution returns that error instead of DELEGATION_REQUIRED, and flow authorize calls the same loader and also fails. Repeating either command leaves the record and run unchanged, so the run cannot progress without manually deleting durable state. This is introduced by the schema bump and mandatory provenance fields. A regression test should use an actual base-version schema-2 record, verify its authority is not reused, then verify the production path requests and accepts a fresh schema-3 authorization.
Confidence: 0.98
Location: boatstack/internal/softwaredelivery/delegation/record.go:124-128 (RIGHT)
[P2] Declarative flow approval is not bound to the presented identity provider
Invariant: a human-authorized declarative state effect must be bound to the exact identity provider/configuration shown at suspension. Suspend a human-required transition under provider A, legitimately change the repository identity to provider B without changing the flow artifact, then resume using the cached actor from A. The changed admission code validates only actor syntax, so it commits the transition; the receipt contains only the actor and no provider fingerprint. This makes provider drift invisible and records authorization under stale context. A regression test should change the descriptor between suspension and retry and assert a drift refusal with an unchanged state revision.
Confidence: 0.94
Location: boatstack/cmd/boatstack-helper/declarative_flow.go:268-275 (RIGHT)
| For a ` + "`literal`" + ` descriptor, use its validated ` + "`value`" + ` as the proposed | ||
| actor. For a ` + "`command`" + ` descriptor, execute the exact ` + "`command`" + ` and | ||
| ` + "`args`" + ` directly through the host command tool. Do not join them into a shell | ||
| string, interpolate values, rewrite arguments, or use a shell evaluator. Require a |
There was a problem hiding this comment.
[P1] Repository identity descriptors can trigger arbitrary host commands
Invariant: repository-authored identity metadata must not authorize effects before kernel admission. A candidate config can declare {"kind":"command","command":"sh","args":["-c","touch marker"]}; the generated trusted skill directs the host to execute it before requesting human approval. This bypass is introduced by the new command-descriptor protocol, and “no shell evaluator” does not help when the selected executable is itself a shell. The command can mutate files or use ambient credentials without admission or receipts. A regression test should expose this descriptor through a program-change suspension using a fake host executor and verify that no command runs and no marker is created without an externally granted provider capability.
Confidence: 0.99
| ) | ||
|
|
||
| const ConfigSchemaVersion = 2 | ||
| const ConfigSchemaVersion = 3 |
There was a problem hiding this comment.
[P1] Schema 3 has no kernel-owned upgrade path from existing installations
Invariant: an admitted installation must have a supported transition to the next runtime/configuration ABI. Every base-revision repository has schema 2; install.sh update invokes the head runtime, whose strict decoder rejects that source before installation.update can resolve or apply. The base runtime also rejects a schema-3 mutation, while the head runtime rejects schema 2, leaving no kernel-owned bridge. Thus all existing installations are unable to upgrade without out-of-band configuration mutation. The regression test must install with the actual base binary, then update with the head binary and verify a committed transition to schema 3—not build current source twice with altered version strings.
Confidence: 0.99
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.94
The patch can emit a delegation suspension whose request and presented identity come from different committed configuration snapshots, and its command-output parser rejects a documented valid boundary value. Model-level verification is recommended before merge, particularly to check whether repeated configuration or installation reprojection can keep delegation on a zero-progress retry path.
Findings without inline diff anchors
[P2] Bind delegation identity to the same request snapshot
Invariant: a delegation suspension's identity provider must match the provider included in its request fingerprint. With detached configuration, bind a request under provider P1/F1, concurrently commit configuration.mutate to P2, then enter delegationRequiredResponse: it rereads P2 but emits the existing F1, producing a mixed suspension that flow authorize deterministically rejects. This patch introduces the mismatch by adding provider identity to delegation fingerprints while independently resolving live identity for the response. The user approves an unusable request and must restart. Add a detached-config concurrency regression asserting the response either retains P1 or refuses drift, never returns F1/P2.
Confidence: 0.93
Location: boatstack/cmd/boatstack-helper/delegation_runtime.go:205-212 (RIGHT)
| if len(stdout) == 0 || len(stdout) > MaxCommandOutputBytes { | ||
| return "", fmt.Errorf("HUMAN_IDENTITY_RESOLUTION_FAILED: stdout must be 1-%d bytes", MaxCommandOutputBytes) | ||
| } | ||
| value := append([]byte(nil), stdout...) | ||
| if bytes.HasSuffix(value, []byte("\r\n")) { | ||
| value = value[:len(value)-2] | ||
| } else if bytes.HasSuffix(value, []byte("\n")) { | ||
| value = value[:len(value)-1] |
There was a problem hiding this comment.
[P3] Apply the output limit after stripping the line ending
Invariant: every valid actor line of at most 1 KiB must be accepted with an optional trailing LF or CRLF. A 1024-byte valid actor followed by \n is rejected because the raw 1025-byte output is bounded before the terminator is removed. This new interpreter therefore rejects output explicitly allowed by the documented host contract. Check the raw limit as MaxActorBytes plus the permitted terminator, then validate the stripped actor; add boundary tests for 1024-byte actors with LF and CRLF.
Confidence: 0.99
There was a problem hiding this comment.
Codex automated review
Verdict: patch is incorrect
Confidence: 0.96
The patch introduces a durable declarative-run failure and a command-provider boundary error. Model-level verification is recommended before merge after fixing these findings, particularly to check whether identity rotation combined with input supersession or delegation recovery can enter a zero-progress cycle.
| invalidAuthority := (receipt.AuthorityContextFingerprint != "" && len(receipt.AuthorityContextFingerprint) != 64) || | ||
| (receipt.HumanActor != "" && (len(receipt.AuthorityContextFingerprint) != 64 || len(receipt.AuthorityFingerprint) != 64)) || | ||
| (receipt.HumanActor == "" && receipt.AuthorityFingerprint != "") |
There was a problem hiding this comment.
[P2] Extra human actor corrupts declarative receipt history
Invariant: every successfully committed declarative receipt must pass the loader's validation. A valid transition requiring no human authority, invoked with the accepted optional --human alice flag, commits HumanActor while both authority fingerprints remain empty; the next replay deterministically rejects that receipt as FLOW_CONTEXT_MISMATCH. This patch introduced that mismatch by requiring fingerprints whenever HumanActor is present while still recording the actor unconditionally. The durable run becomes unreadable after reported success. Add a regression test that executes and then reloads a no-authority declarative transition with --human.
Confidence: 0.99
| if len(stdout) == 0 || len(stdout) > MaxCommandOutputBytes { | ||
| return "", fmt.Errorf("HUMAN_IDENTITY_RESOLUTION_FAILED: stdout must be 1-%d bytes", MaxCommandOutputBytes) |
There was a problem hiding this comment.
[P3] Validate command-output length after removing its terminator
Invariant: command and literal providers must accept the same valid actor range. A 1024-byte valid actor is accepted by ValidateActor, but conventional output containing that actor plus \n is rejected because the 1024-byte limit is checked before stripping the terminator. This newly added interpreter therefore rejects a valid maximum-length actor and contradicts its documented line contract. Test InterpretCommandOutput with MaxActorBytes valid characters followed by LF and CRLF.
Confidence: 0.94
1 participant
Summary
Verification
go test ./...npm run test:flow-sdkpython3 .github/tests/test_repository_contract.pypublished-pr