mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-09-11 02:14:29 +00:00
Address CodeRabbit follow-up review fixes
This commit is contained in:
@@ -14,15 +14,9 @@ Core domains also have a review scope. **Active contract** domains are wired to
|
||||
| audio-monitoring | provider-coordinator | sensitive | inspect, list-providers, register-provider, unregister-provider, select-provider, start, stop, set-direct-monitor | monitoring.start, monitoring.stop, monitoring.status, monitoring.set-direct-monitor | Inspect/list/select/status are prompt-free. Fresh monitoring start requires explicit user action; background requesters may only attach to an active compatible session. Outcomes distinguish handled, stopped, denied, unavailable, degraded, failed, no-owner, no-handler, unsupported-command, incompatible, incompatible-version, provider-selection-required, and user-action-required. Diagnostics redact raw device labels, hardware ids, paths, secrets, live handles, buffers, samples, waveforms, and recordings. |
|
||||
| stems | coordinator plus plugin provider | safe | inspect, mute, restore | stem.get-state, stem.apply-automation, stem.restore-automation | Core coordinates claims/overrides; the active Stems provider owns actual stem state/playback. |
|
||||
| playback | exclusive-owner | safe | inspect, start, pause, resume, stop, seek, set-loop, clear-loop, register-requester, register-observer | none | Core owns the transport control plane while `app.js` keeps raw media handles private. Fresh audible starts require explicit user action. Diagnostics expose pseudonymous targets, sanitized route/timing/loop state, requester/observer summaries, bridge hits, bounded recent outcomes, and no audio elements, native handles, decoded buffers, samples, waveforms, or recordings. |
|
||||
| progression | exclusive-owner | safe | inspect, record-event, list-shop, buy-item, equip-item | none | Core owns mastery rank, the challenge/quest engine, the Decibels wallet, and the cosmetics shop (spec 010). `record-event` accepts whitelisted types only (`minigame_run`); `song_completed` is server-derived in `/api/stats` and denied here. `buy-item`/`equip-item` require explicit user action. Decibels are play-earned only — no real-money path exists or may be added. Diagnostics (`slopsmith.progression.diag.v1`) carry content warnings, rank/level/quest counts, and wallet totals; no song filenames or display names. |
|
||||
|
||||
| jobs | provider-coordinator | privileged | register-provider, unregister-provider, list-providers, enqueue, list, inspect, cancel, pause, resume, retry, record-bridge-hit | job.enqueue, job.status, job.cancel, job.pause, job.resume, job.retry, job.recover | Core coordinates provider selection, capacity scheduling, lifecycle state, recovery references, bridge hits, and diagnostics. Fresh privileged enqueue/retry requires explicit user approval or a matching approved-continuation scope. List/inspect are prompt-free and side-effect-free. Diagnostics redact paths, filenames, URLs, tokens, command lines, media/artifacts, recordings, handles, and provider-private payloads. |
|
||||
| audio-effects | provider-coordinator | sensitive | inspect, list-providers, register-provider, unregister-provider, select-chain, resolve-plan, inspect-route, bypass, restore, fallback, activate-segment, set-stage-bypass, set-stage-parameter, record-bridge-hit | chain.resolve, chain.inspect, segment.activate, stage.set-bypass, stage.set-parameter, route.bypass, route.restore | Core owns provider selection, route state, chain-plan schema validation, fallback accounting, and diagnostics. Providers propose opaque NAM/IR/VST/utility chain plans; trusted desktop/native code validates and loads processors. Chain selection and route bypass/restore require explicit user action or restored selection. Diagnostics omit raw paths, filenames, URLs, model/IR names, native preset JSON, VST state blobs, handles, callbacks, DOM nodes, audio buffers, samples, and waveforms. |
|
||||
|
||||
| visualization | provider-coordinator | safe | inspect, list-providers, select-renderer, clear-renderer | renderer.create, renderer.destroy | Highway renderer provider registry, picker-delegated selection, auto-match attribution, and failure fallback. `renderer.create` maps to the legacy `window.slopsmithViz_*` factory `init(canvas, ctx)` call; `renderer.destroy` maps to the factory `destroy()` teardown. Legacy `type: "visualization"` manifests and `window.slopsmithViz_*` globals are accounted compatibility shims. Diagnostics carry provider ids/labels, selection source, last auto-match outcome, and last failure — no song filenames, titles, or arrangement names. |
|
||||
|
||||
| note-detection | provider-coordinator | sensitive | inspect, register-provider, unregister-provider, open-binding, close-binding, set-target, clear-target | pitch.estimate, verify.target | Detection-binding control plane (spec 009): providers (midi/engine/js) serve primitives; each requester binds its own redacted tuning context; consumers own judgment, hit/miss flow as observability events. Legacy `highway.setNoteStateProvider` is an accounted shim. Diagnostics carry provider/binding summaries and bounded outcomes — no raw audio, sample data, device labels, or song identity. |
|
||||
|
||||
Privileged commands are roadmap-only until they have: a visible user confirmation path, diagnostics redaction rules, failure recovery, and tests that prove disabled or incompatible participants cannot execute handlers.
|
||||
|
||||
## Expected Future Domains
|
||||
@@ -34,6 +28,8 @@ These domains are expected future capability contracts, not current runtime grap
|
||||
| ui.navigation | exclusive-owner | safe | register-contribution, mount, unmount, set-visible, reorder-by-policy, navigate, inspect | Needs a UI host PR with contribution placement and route/screen semantics. |
|
||||
| ui.plugin-screens | exclusive-owner | safe | register-contribution, mount, unmount, set-visible, reorder-by-policy, inspect | Needs a screen host PR with mount/unmount and visibility policy. |
|
||||
| settings | exclusive-owner | sensitive | register-contribution, mount, unmount, set-visible, reorder-by-policy, inspect | Needs redaction rules and a migration story for settings metadata. |
|
||||
| visualization | multi-provider | safe | register-provider, get-current, set-renderer | Needs provider ordering/selection rules and legacy highway shim attribution. |
|
||||
| note-detection | multi-provider | sensitive | register, inspect | Needs performance-data redaction and provider lifecycle tests. |
|
||||
| backend.routes | multi-provider | privileged | register, inspect | Needs a concrete backend route/provider workflow, privilege review, and route diagnostics. |
|
||||
| ui.player-controls | exclusive-owner | safe | register-contribution, mount, unmount, set-visible, reorder-by-policy, inspect | Needs a first-party player-control host. |
|
||||
| ui.player-panels | exclusive-owner | safe | register-contribution, mount, unmount, set-visible, reorder-by-policy, inspect | Needs a first-party panel host and layout policy. |
|
||||
|
||||
@@ -2448,7 +2448,7 @@ class TuningProviderRegistry:
|
||||
for name, freqs in names.items():
|
||||
result[instrument][name] = [round(f * scale, 4) for f in freqs]
|
||||
except Exception:
|
||||
logger.exception("tuning provider %r raised during get_merged()", provider_id)
|
||||
log.exception("tuning provider %r raised during get_merged()", provider_id)
|
||||
return result
|
||||
|
||||
|
||||
@@ -4583,7 +4583,8 @@ def api_award_xp(data: dict):
|
||||
# bind (→ 500) and no real run awards anywhere near this.
|
||||
if not (0 <= amount <= 10_000_000):
|
||||
return JSONResponse({"error": "amount must be between 0 and 10,000,000"}, status_code=400)
|
||||
meta_db.award_xp(amount)
|
||||
source = _clean_str(data.get("source")) or None
|
||||
meta_db.award_xp(amount, source)
|
||||
return meta_db.get_progress()
|
||||
|
||||
|
||||
@@ -6332,9 +6333,9 @@ async def api_jobs_cancel(job_id: str, data: dict | None = Body(default=None)):
|
||||
result = backend_jobs.inspect(job_id)
|
||||
if result.get("outcome") == "no-target":
|
||||
raise HTTPException(status_code=404, detail=result.get("reason") or "job not found")
|
||||
request = data if isinstance(data, dict) else {}
|
||||
request.setdefault("authorization", "user-action")
|
||||
request.setdefault("requesterId", "api.jobs")
|
||||
request = dict(data) if isinstance(data, dict) else {}
|
||||
request["authorization"] = "user-action"
|
||||
request["requesterId"] = "api.jobs"
|
||||
return await backend_jobs.dispatch_action(job_id, "job.cancel", request)
|
||||
|
||||
|
||||
@@ -6343,9 +6344,9 @@ async def api_jobs_retry(job_id: str, data: dict | None = Body(default=None)):
|
||||
result = backend_jobs.inspect(job_id)
|
||||
if result.get("outcome") == "no-target":
|
||||
raise HTTPException(status_code=404, detail=result.get("reason") or "job not found")
|
||||
request = data if isinstance(data, dict) else {}
|
||||
request.setdefault("authorization", "user-action")
|
||||
request.setdefault("requesterId", "api.jobs")
|
||||
request = dict(data) if isinstance(data, dict) else {}
|
||||
request["authorization"] = "user-action"
|
||||
request["requesterId"] = "api.jobs"
|
||||
return await backend_jobs.dispatch_action(job_id, "job.retry", request)
|
||||
|
||||
|
||||
@@ -6375,7 +6376,22 @@ async def jobs_ws(websocket: WebSocket):
|
||||
try:
|
||||
await websocket.send_json({"type": "snapshot", "snapshot": backend_jobs.snapshot()})
|
||||
while True:
|
||||
event = await queue.get()
|
||||
event_task = asyncio.create_task(queue.get())
|
||||
recv_task = asyncio.create_task(websocket.receive())
|
||||
done, pending = await asyncio.wait(
|
||||
{event_task, recv_task},
|
||||
return_when=asyncio.FIRST_COMPLETED,
|
||||
)
|
||||
for task in pending:
|
||||
task.cancel()
|
||||
if pending:
|
||||
await asyncio.gather(*pending, return_exceptions=True)
|
||||
if recv_task in done:
|
||||
message = recv_task.result()
|
||||
if message.get("type") == "websocket.disconnect":
|
||||
break
|
||||
continue
|
||||
event = event_task.result()
|
||||
await websocket.send_json(event)
|
||||
except WebSocketDisconnect:
|
||||
pass
|
||||
|
||||
@@ -64,6 +64,27 @@ Required args:
|
||||
|
||||
Outcomes: `handled`, `validation-failed`, `incompatible-version`, `denied`.
|
||||
|
||||
### `unregister-provider`
|
||||
|
||||
Unregisters a provider and settles provider-owned active jobs to terminal state.
|
||||
|
||||
Required args:
|
||||
|
||||
```json
|
||||
{
|
||||
"providerId": "string"
|
||||
}
|
||||
```
|
||||
|
||||
Rules:
|
||||
|
||||
- Command is privileged and host-owned.
|
||||
- Missing/unknown provider returns `no-owner`.
|
||||
- Registered provider emits `provider-unregistered`.
|
||||
- Any active jobs owned by the provider transition to terminal unavailable/orphaned state with a safe reason.
|
||||
|
||||
Outcomes: `handled`, `no-owner`, `failed`.
|
||||
|
||||
### `list-providers`
|
||||
|
||||
Returns redaction-safe provider summaries. Prompt-free and side-effect-free.
|
||||
@@ -92,7 +113,7 @@ Rules:
|
||||
|
||||
- Privileged work requires `authorization: "user-action"` or an approved continuation matching provider, job type, target, requester, and inputs.
|
||||
- Missing or mismatched approval returns `denied` or `user-action-required` before provider operation callbacks run.
|
||||
- If exactly one compatible provider exists, it may be selected automatically.
|
||||
- If exactly one compatible provider exists, it MUST be selected automatically.
|
||||
- If multiple compatible providers exist, `providerId` or selected/default provider is required; otherwise return `provider-selection-required`.
|
||||
- Validation happens before provider work starts.
|
||||
- Provider capacity controls whether the job enters `queued` or `running`.
|
||||
|
||||
@@ -121,8 +121,8 @@ A user or maintainer troubleshooting long-running work can tell whether a job fa
|
||||
- **FR-004**: System MUST support job states including queued, running, paused, cancellation-requested, cancelled, completed, failed, provider-unavailable, and orphaned.
|
||||
- **FR-005**: System MUST expose explicit outcomes including handled, queued, denied, user-action-required, unavailable, no-owner, no-handler, no-target, unsupported-command, unsupported-operation, incompatible, incompatible-version, provider-selection-required, validation-failed, stale, cancelled, completed, failed, timeout, and retry-started.
|
||||
- **FR-006**: System MUST allow users and authorized requesters to inspect current jobs and providers without triggering privileged work, file writes, downloads, subprocesses, or external-service calls.
|
||||
- **FR-007**: System MUST require explicit user action before enqueueing any job that can create, modify, delete, download, export, convert, or publish user-visible files or plugin state; approval applies only to one job request and provider-declared retry or continuation attempts for the same provider, job type, target, and requester.
|
||||
- **FR-008**: System MUST reject background or plugin-initiated privileged jobs without user approval using a distinct denied or user-action-required outcome.
|
||||
- **FR-007**: System MUST require explicit user action before enqueueing any job that can create, modify, delete, download, export, convert, or publish user-visible files or plugin state; approval applies only to one job request and provider-declared retry or continuation attempts for the same provider, job type, target, requester, and inputs.
|
||||
- **FR-008**: System MUST reject background or plugin-initiated privileged jobs without user approval using a distinct denied or user-action-required outcome, including approved-continuation attempts whose inputs do not match the original approved scope.
|
||||
- **FR-009**: System MUST apply provider-declared scheduling limits so queued jobs do not exceed provider capacity and running jobs do not silently overrun declared concurrency; user-approved interactive jobs MUST run before background or maintenance jobs, with FIFO ordering within each priority.
|
||||
- **FR-010**: System MUST make provider registration idempotent so repeated hydration updates provider state without duplicate providers or duplicated active jobs.
|
||||
- **FR-011**: System MUST auto-select only when exactly one compatible provider can handle the job type; when multiple compatible providers exist, the system MUST use a user-selected/default provider or return provider-selection-required.
|
||||
|
||||
@@ -91,13 +91,13 @@ test('provider-private enqueue payload reaches only the provider callback', asyn
|
||||
await dispatch(window, 'register-provider', { provider });
|
||||
|
||||
const result = await dispatch(window, 'enqueue', enqueuePayload({
|
||||
providerPayload: { filename: '/Users/example/DLC/Secret Song.psarc', token: 'abc123' },
|
||||
providerPayload: { filename: '/Users/example/DLC/Secret Song.sloppak', token: 'abc123' },
|
||||
target: { safeRef: 'target-secret-song' },
|
||||
inputs: { safeFingerprint: 'input-secret-song' },
|
||||
}));
|
||||
|
||||
assert.equal(result.status, 'applied');
|
||||
assert.deepEqual(privatePayload, { filename: '/Users/example/DLC/Secret Song.psarc', token: 'abc123' });
|
||||
assert.deepEqual(privatePayload, { filename: '/Users/example/DLC/Secret Song.sloppak', token: 'abc123' });
|
||||
assert.doesNotMatch(JSON.stringify(result.payload.job), /Secret Song|abc123|filename/);
|
||||
assert.doesNotMatch(JSON.stringify(diagnosticsSnapshot(window)), /Secret Song|abc123|filename/);
|
||||
});
|
||||
|
||||
@@ -27,7 +27,7 @@ def test_backend_jobs_adopt_progress_and_complete_redacts_raw_payloads():
|
||||
progressed = jobs.update_progress(
|
||||
"provider.test",
|
||||
"backend-1",
|
||||
{"mode": "determinate", "percent": 42, "step": "convert", "message": "/Users/example/Secret.psarc token=abc"},
|
||||
{"mode": "determinate", "percent": 42, "step": "convert", "message": "/Users/example/Secret.sloppak token=abc"},
|
||||
)
|
||||
assert progressed["outcome"] == "handled"
|
||||
|
||||
@@ -38,7 +38,7 @@ def test_backend_jobs_adopt_progress_and_complete_redacts_raw_payloads():
|
||||
text = str(snapshot)
|
||||
assert "provider.test" in text
|
||||
assert "target-abc" in text
|
||||
assert "Secret.psarc" not in text
|
||||
assert "Secret.sloppak" not in text
|
||||
assert "token=abc" not in text
|
||||
assert "/Users/example" not in text
|
||||
assert snapshot["jobs"]["recentTerminal"][0]["state"] == "completed"
|
||||
@@ -70,7 +70,7 @@ def test_backend_jobs_dispatches_private_provider_action_and_redacts_payload():
|
||||
"outcome": "cancelled",
|
||||
"payload": {
|
||||
"jobId": request["job"]["jobId"],
|
||||
"rawPath": "/Users/example/Secret.psarc",
|
||||
"rawPath": "/Users/example/Secret.sloppak",
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user