Repository navigation
[3266 by Claude Opus 5.5] Add bounded phase diagnostics to protected signed RPM acceptance failures - #3267
Conversation
…ailed and how: an exit status, signal, timeout, spawn error code, or interruption. The new failure-injection tests pass. I did not touch the existing run's failure, and nothing here identifies its cause.
What's established about run 39305432099:
- Identity, helper verification and signing passed, and the job then failed in the signed RPM acceptance step.
- The old runner discarded child stderr and threw the same message for every non-zero subprocess.
- So the cause could be a failed image build, a failed isolated network setup, or a failure inside the installed package/browser container. All three are equally consistent with that log.
What remains unknown: which of those phases failed, its exit status, and the root cause. No reproduction evidence has been provided yet, so I made no speculative fix. The next run will name the phase.
What changed
- New `apps/desktop/scripts/signed-rpm-acceptance-diagnostics.mjs`:
- Fixed names only. Every failure is a `SignedRpmAcceptanceFailure` built only from allowlisted fields: a fixed phase name, an outcome, a numeric exit status (0–255), a Node signal name, an `E…` spawn code, and a container step from a fixed list. Anything else becomes `unknown`/`error`.
- Nothing from the child. Messages and diagnostics never contain child stdout/stderr, arguments, environment values, paths, key material or a `cause`.
- `run-signed-installed-rpm-acceptance.mjs`:
- Phases. Each step runs under a fixed phase: `docker-info`, `workspace-setup`, `context-preparation`, `image-build`, `network-create`, `installed-acceptance`, `evidence-collection`, `evidence-export`, `evidence-write`.
- Cleanup. All owned cleanups (container, network inspect/rm, image, temp workspace) now run even if an earlier one fails. Cleanup failures are listed under `cleanup` after the original failure; previously the old nested `finally` could replace the original error.
- Evidence-collection errors. These are reduced to the phase name only. Node's JSON parse errors can quote file content, and file errors include paths.
- CLI output. On failure it prints one bounded message plus one JSON line, e.g. `{"signedRpmAcceptanceFailure":{"phase":"installed-acceptance","outcome":"exit","exitCode":1,"stage":"package-install"}}`, and exits 1.
- `test-signed-installed-rpm.sh`: the container script now prints a fixed marker on entering each step (`key-trust`, `signature-verify`, `package-install`, `installed-verify`, `runtime-manifest`, `browser-acceptance`, `evidence-export`, `post-verify`, …). The runner keeps only the last complete marker line that matches the allowlist and throws away all other output. This lets the report separate the package install from the browser run.
Behaviour you'd notice:
- The failure message now reads `Signed installed RPM acceptance failed; no success evidence emitted [phase=… outcome=…]` instead of the old fixed text.
- Raw spawn errors, which included the path, and the old "interrupted" message are replaced by the bounded form.
- A signal now counts as a failure even for commands that were allowed to fail.
- The function takes optional `docker`, `verify` and `prepareContext` substitutes so the tests can drive it. The CLI always uses the real Docker CLI, real RPM verification and the allowlisted context.
Signature, source and runtime identity checks, the internal network, the no-success-evidence rule, permissions, the workflow and artifact uploads are all unchanged.
Tests (`signed-rpm-acceptance-diagnostics.test.mjs`, 12 tests, also in the desktop `npm test` glob):
- Unit tests: exit, signal, timeout and spawn outcomes, plus marker parsing with split, garbage, spoofed and too-long lines.
- Full-flow runs: these use a fake Docker CLI that echoes a sentinel secret, its arguments and its environment on every output stream. They cover:
- each major phase failing, plus a complete passing run.
- the primary failure surviving three failing cleanups, while a network it doesn't own is not removed.
- context, evidence and artifact-changed failures.
- Leak checks: the sentinel and paths are checked never to appear in messages, diagnostics or `inspect()` output.
- Mutation checks: dropping the cleanup merge made the tests fail, and so did loosening the stage allowlist.
- Existing suites: the evidence, workflow, context and RPM signing tests pass (32 pass), but 4 tests were skipped because GnuPG and the RPM tools aren't installed here. So the real-tool signing tests need to run in CI or with operator validation.
- Lint: eslint passes with zero warnings.
- Not run: no real Docker, owner secrets or protected dispatch were used.
I also added a section on the diagnostic format to `docs/docs/operations/linux-rpm-signing.md`. No visual preview applies.
|
/review gpt-6-astra |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — gpt-6-astraOverall EvaluationThe PR adds useful, bounded phase diagnostics while preserving the acceptance checks and prioritizing the original failure over cleanup errors. It is merge-ready within the supplied scope, subject to pending checks completing successfully. ✅ Controlled diagnostic output — Failure messages are rebuilt from restricted fields; child stdout, stderr, arguments, and environment values are not included. ✅ Primary failure preservation — Cleanup operations run independently, and their failures are appended without replacing the substantive acceptance failure. ✅ Focused failure injection — Tests cover subprocess failures, stage parsing, evidence rejection, privacy, and continued cleanup. This was a static review; no commands were run. The supplied issue description is explicitly truncated, so unseen acceptance requirements could not be assessed. The supplied diff was reviewed as complete. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Test parent interruption explicitlyThe new runner classifies parent S2: 🟢 Clarify tolerated cleanup failuresThe runner retains existing ScoreThe implementation addresses the visible objective with focused tests and no verified introduced blocker. Review confidence is limited to static analysis; the authoritative check summary reports no failures and 15 pending checks. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3266
Branch:
3266/claude-opus-5-5-add-bounded-phase-diagnos-20261019-1857-9wqCommits: ✅ Changes committed (7a4eab8)
AI Processing Completed
Execution Details:
Summary:
A failed protected acceptance job will now report which fixed phase failed and how: an exit status, signal, timeout, spawn error code, or interruption. The new failure-injection tests pass. I did not touch the existing run's failure, and nothing here identifies its cause.
What's established about run 39305432099:
What remains unknown: which of those phases failed, its exit status, and the root cause. No reproduction evidence has been provided yet, so I made no speculative fix. The next run will name the phase.
What changed
apps/desktop/scripts/signed-rpm-acceptance-diagnostics.mjs:SignedRpmAcceptanceFailurebuilt only from allowlisted fields: a fixed phase name, an outcome, a numeric exit status (0–255), a Node signal name, anE…spawn code, and a container step from a fixed list. Anything else becomesunknown/error.cause.run-signed-installed-rpm-acceptance.mjs:docker-info,workspace-setup,context-preparation,image-build,network-create,installed-acceptance,evidence-collection,evidence-export,evidence-write.cleanupafter the original failure; previously the old nestedfinallycould replace the original error.{"signedRpmAcceptanceFailure":{"phase":"installed-acceptance","outcome":"exit","exitCode":1,"stage":"package-install"}}, and exits 1.test-signed-installed-rpm.sh: the container script now prints a fixed marker on entering each step (key-trust,signature-verify,package-install,installed-verify,runtime-manifest,browser-acceptance,evidence-export,post-verify, …). The runner keeps only the last complete marker line that matches the allowlist and throws away all other output. This lets the report separate the package install from the browser run.Behaviour you'd notice:
Signed installed RPM acceptance failed; no success evidence emitted [phase=… outcome=…]instead of the old fixed text.docker,verifyandprepareContextsubstitutes so the tests can drive it. The CLI always uses the real Docker CLI, real RPM verification and the allowlisted context.Signature, source and runtime identity checks, the internal network, the no-success-evidence rule, permissions, the workflow and artifact uploads are all unchanged.
Tests (
signed-rpm-acceptance-diagnostics.test.mjs, 12 tests, also in the desktopnpm testglob):inspect()output.I also added a section on the diagnostic format to
docs/docs/operations/linux-rpm-signing.md. No visual preview applies.Detailed Logs:
532066ba-8810-428f-84e7-f2c3756b8cf6Log files stored at:
/usr/src/app/logs/executions/issue-3266-2026-10-19T19-06-36-273Z-conversation.json/usr/src/app/logs/executions/issue-3266-2026-10-19T19-06-36-273Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3266.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!